RE: [PATCH v5 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

[Reshetova, Elena]

> * Elena Reshetova <elena.reshetova@xxxxxxxxx> wrote:
> 
> > @@ -19,10 +19,15 @@ static int sgx_open(struct inode *inode, struct file
> *file)
> >  	struct sgx_encl *encl;
> >  	int ret;
> >
> > +	ret = sgx_inc_usage_count();
> > +	if (ret)
> > +		return -EBUSY;
> 
> So if sgx_inc_usage_count() returns nonzero, it's in use already and we
> return -EBUSY, right?

I guess my selection of error code here was wrong. 
The intended logic is if sgx_inc_usage_count() returns nonzero,
the *incrementing of counter failed* (due to failed EUPDATESVN)
and we want to stop and report error.
  
> 
> But:
> 
> >  int sgx_inc_usage_count(void)
> >  {
> > +	int ret;
> > +
> > +	/*
> > +	 * Increments from non-zero indicate EPC other
> > +	 * active EPC users and EUPDATESVN is not attempted.
> > +	 */
> > +	if (atomic64_inc_not_zero(&sgx_usage_count))
> > +		return 0;
> 
> If sgx_usage_count is 1 here (ie. it's busy), this will return *zero*,
> and sgx_open() will not run into the -EBUSY condition and will continue
> assuming it has claimed the usage count, while it hasn't ...

Yes, meaning is different, see above. 

> 
> Furthermore, in the sgx_open() error paths we can then run into

What error paths? In case sgx_inc_usage_count() fails, we exit
immediately. 

> sgx_dec_usage_count(), which will merrily underflow the counter into
> negative:
> 
>  +void sgx_dec_usage_count(void)
>  +{
>  +       atomic64_dec(&sgx_usage_count);
>  +}
> 
> How is this all supposed to work?

Looks like I need more explanation on the counter and a better error
returned to userspace than -EBUSY. Maybe EIO then? 

Best Regards,
Elena.

> 
> Thanks,
> 
> 	Ingo