[PATCH v6] usb: f_fs: Fix use-after-free for epfile

[PATCH v6] usb: f_fs: Fix use-after-free for epfile

[Udipto Goswami]

Consider a case where ffs_func_eps_disable is called from
ffs_func_disable as part of composition switch and at the
same time ffs_epfile_release get called from userspace.
ffs_epfile_release will free up the read buffer and call
ffs_data_closed which in turn destroys ffs->epfiles and
mark it as NULL. While this was happening the driver has
already initialized the local epfile in ffs_func_eps_disable
which is now freed and waiting to acquire the spinlock. Once
spinlock is acquired the driver proceeds with the stale value
of epfile and tries to free the already freed read buffer
causing use-after-free.

Following is the illustration of the race:

      CPU1                                  CPU2

   ffs_func_eps_disable
   epfiles (local copy)
					ffs_epfile_release
					ffs_data_closed
					if (last file closed)
					ffs_data_reset
					ffs_data_clear
					ffs_epfiles_destroy
spin_lock
dereference epfiles

Fix this races by taking epfiles local copy & assigning it under
spinlock and if epfiles(local) is null then update it in ffs->epfiles
then finally destroy it.

Reviewed-by: John Keeping <john@metanate.com>
Signed-off-by: Pratham Pratap <quic_ppratap@quicinc.com>
Co-developed-by: Udipto Goswami <quic_ugoswami@quicinc.com>
Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
---
v6: Addressing minor code formatting.

 drivers/usb/gadget/function/f_fs.c | 44 +++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 3c584da..6f23a66 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1711,16 +1711,24 @@ static void ffs_data_put(struct ffs_data *ffs)
 
 static void ffs_data_closed(struct ffs_data *ffs)
 {
+	struct ffs_epfile *epfiles;
+	unsigned long flags;
+
 	ENTER();
 
 	if (atomic_dec_and_test(&ffs->opened)) {
 		if (ffs->no_disconnect) {
 			ffs->state = FFS_DEACTIVATED;
-			if (ffs->epfiles) {
-				ffs_epfiles_destroy(ffs->epfiles,
-						   ffs->eps_count);
-				ffs->epfiles = NULL;
-			}
+			spin_lock_irqsave(&ffs->eps_lock, flags);
+			epfiles = ffs->epfiles;
+			ffs->epfiles = NULL;
+			spin_unlock_irqrestore(&ffs->eps_lock,
+							flags);
+
+			if (epfiles)
+				ffs_epfiles_destroy(epfiles,
+						 ffs->eps_count);
+
 			if (ffs->setup_state == FFS_SETUP_PENDING)
 				__ffs_ep0_stall(ffs);
 		} else {
@@ -1767,14 +1775,27 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
 
 static void ffs_data_clear(struct ffs_data *ffs)
 {
+	struct ffs_epfile *epfiles;
+	unsigned long flags;
+
 	ENTER();
 
 	ffs_closed(ffs);
 
 	BUG_ON(ffs->gadget);
 
-	if (ffs->epfiles)
-		ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count);
+	spin_lock_irqsave(&ffs->eps_lock, flags);
+	epfiles = ffs->epfiles;
+	ffs->epfiles = NULL;
+	spin_unlock_irqrestore(&ffs->eps_lock, flags);
+
+	/*
+	 * potential race possible between ffs_func_eps_disable
+	 * & ffs_epfile_release therefore maintaining a local
+	 * copy of epfile will save us from use-after-free.
+	 */
+	if (epfiles)
+		ffs_epfiles_destroy(epfiles, ffs->eps_count);
 
 	if (ffs->ffs_eventfd)
 		eventfd_ctx_put(ffs->ffs_eventfd);
@@ -1919,12 +1940,15 @@ static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count)
 
 static void ffs_func_eps_disable(struct ffs_function *func)
 {
-	struct ffs_ep *ep         = func->eps;
-	struct ffs_epfile *epfile = func->ffs->epfiles;
-	unsigned count            = func->ffs->eps_count;
+	struct ffs_ep *ep;
+	struct ffs_epfile *epfile;
+	unsigned short count;
 	unsigned long flags;
 
 	spin_lock_irqsave(&func->ffs->eps_lock, flags);
+	count = func->ffs->eps_count;
+	epfile = func->ffs->epfiles;
+	ep = func->eps;
 	while (count--) {
 		/* pending requests get nuked */
 		if (ep->ep)
-- 
2.7.4

Re: [PATCH v6] usb: f_fs: Fix use-after-free for epfile

[Greg Kroah-Hartman]

On Tue, Dec 07, 2021 at 11:37:54AM +0530, Udipto Goswami wrote:
> Consider a case where ffs_func_eps_disable is called from
> ffs_func_disable as part of composition switch and at the
> same time ffs_epfile_release get called from userspace.
> ffs_epfile_release will free up the read buffer and call
> ffs_data_closed which in turn destroys ffs->epfiles and
> mark it as NULL. While this was happening the driver has
> already initialized the local epfile in ffs_func_eps_disable
> which is now freed and waiting to acquire the spinlock. Once
> spinlock is acquired the driver proceeds with the stale value
> of epfile and tries to free the already freed read buffer
> causing use-after-free.
> 
> Following is the illustration of the race:
> 
>       CPU1                                  CPU2
> 
>    ffs_func_eps_disable
>    epfiles (local copy)
> 					ffs_epfile_release
> 					ffs_data_closed
> 					if (last file closed)
> 					ffs_data_reset
> 					ffs_data_clear
> 					ffs_epfiles_destroy
> spin_lock
> dereference epfiles
> 
> Fix this races by taking epfiles local copy & assigning it under
> spinlock and if epfiles(local) is null then update it in ffs->epfiles
> then finally destroy it.
> 
> Reviewed-by: John Keeping <john@metanate.com>
> Signed-off-by: Pratham Pratap <quic_ppratap@quicinc.com>
> Co-developed-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> ---
> v6: Addressing minor code formatting.
> 
>  drivers/usb/gadget/function/f_fs.c | 44 +++++++++++++++++++++++++++++---------
>  1 file changed, 34 insertions(+), 10 deletions(-)

What commit does this fix?

Does this need to go to stable kernel releases?

thanks,

greg k-h


> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 3c584da..6f23a66 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -1711,16 +1711,24 @@ static void ffs_data_put(struct ffs_data *ffs)
>  
>  static void ffs_data_closed(struct ffs_data *ffs)
>  {
> +	struct ffs_epfile *epfiles;
> +	unsigned long flags;
> +
>  	ENTER();
>  
>  	if (atomic_dec_and_test(&ffs->opened)) {
>  		if (ffs->no_disconnect) {
>  			ffs->state = FFS_DEACTIVATED;
> -			if (ffs->epfiles) {
> -				ffs_epfiles_destroy(ffs->epfiles,
> -						   ffs->eps_count);
> -				ffs->epfiles = NULL;
> -			}
> +			spin_lock_irqsave(&ffs->eps_lock, flags);
> +			epfiles = ffs->epfiles;
> +			ffs->epfiles = NULL;
> +			spin_unlock_irqrestore(&ffs->eps_lock,
> +							flags);
> +
> +			if (epfiles)
> +				ffs_epfiles_destroy(epfiles,
> +						 ffs->eps_count);

You are accessing epfiles outside of the lock.  How is that ok?

> +
>  			if (ffs->setup_state == FFS_SETUP_PENDING)
>  				__ffs_ep0_stall(ffs);
>  		} else {
> @@ -1767,14 +1775,27 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
>  
>  static void ffs_data_clear(struct ffs_data *ffs)
>  {
> +	struct ffs_epfile *epfiles;
> +	unsigned long flags;
> +
>  	ENTER();
>  
>  	ffs_closed(ffs);
>  
>  	BUG_ON(ffs->gadget);
>  
> -	if (ffs->epfiles)
> -		ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count);
> +	spin_lock_irqsave(&ffs->eps_lock, flags);
> +	epfiles = ffs->epfiles;
> +	ffs->epfiles = NULL;
> +	spin_unlock_irqrestore(&ffs->eps_lock, flags);
> +
> +	/*
> +	 * potential race possible between ffs_func_eps_disable
> +	 * & ffs_epfile_release therefore maintaining a local
> +	 * copy of epfile will save us from use-after-free.
> +	 */
> +	if (epfiles)
> +		ffs_epfiles_destroy(epfiles, ffs->eps_count);

How will it save you from a use-after-free as you never increased a
reference count on the pointer here?

And don't write code with new races.  Or are you trying to say this
fixes a race?

Just saving off a pointer feels wrong here.  What happens if the lock is
properly grabbed, then freed, right after you unlock?  You aren't really
protecting this properly in a normal way that reference counts and locks
work from what I can tell, only trying to reduce the race window but not
fixing it.

thanks,

greg k-h

Re: [PATCH v6] usb: f_fs: Fix use-after-free for epfile

[Udipto Goswami]

Hi Greg,

Apologies for the delay, please find the following comments as per my 
understanding inline.

On 13-12-2021 07:38 pm, Greg Kroah-Hartman wrote:
> On Tue, Dec 07, 2021 at 11:37:54AM +0530, Udipto Goswami wrote:
>> Consider a case where ffs_func_eps_disable is called from
>> ffs_func_disable as part of composition switch and at the
>> same time ffs_epfile_release get called from userspace.
>> ffs_epfile_release will free up the read buffer and call
>> ffs_data_closed which in turn destroys ffs->epfiles and
>> mark it as NULL. While this was happening the driver has
>> already initialized the local epfile in ffs_func_eps_disable
>> which is now freed and waiting to acquire the spinlock. Once
>> spinlock is acquired the driver proceeds with the stale value
>> of epfile and tries to free the already freed read buffer
>> causing use-after-free.
>>
>> Following is the illustration of the race:
>>
>>        CPU1                                  CPU2
>>
>>     ffs_func_eps_disable
>>     epfiles (local copy)
>> 					ffs_epfile_release
>> 					ffs_data_closed
>> 					if (last file closed)
>> 					ffs_data_reset
>> 					ffs_data_clear
>> 					ffs_epfiles_destroy
>> spin_lock
>> dereference epfiles
>>
>> Fix this races by taking epfiles local copy & assigning it under
>> spinlock and if epfiles(local) is null then update it in ffs->epfiles
>> then finally destroy it.
Fixes: a9e6f83c2df (usb: gadget: f_fs: stop sleeping in 
ffs_func_eps_disable)
>> Reviewed-by: John Keeping <john@metanate.com>
>> Signed-off-by: Pratham Pratap <quic_ppratap@quicinc.com>
>> Co-developed-by: Udipto Goswami <quic_ugoswami@quicinc.com>
>> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
>> ---
>> v6: Addressing minor code formatting.
>>
>>   drivers/usb/gadget/function/f_fs.c | 44 +++++++++++++++++++++++++++++---------
>>   1 file changed, 34 insertions(+), 10 deletions(-)
> What commit does this fix?
>
> Does this need to go to stable kernel releases?
>
> thanks,
>
> greg k-h
>
This is fixing Commit:  a9e6f83c2df (usb: gadget: f_fs: stop sleeping in 
ffs_func_eps_disable)

Yes this fix is needed for >=5.4 Kernel releases.

>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 3c584da..6f23a66 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -1711,16 +1711,24 @@ static void ffs_data_put(struct ffs_data *ffs)
>>   
>>   static void ffs_data_closed(struct ffs_data *ffs)
>>   {
>> +	struct ffs_epfile *epfiles;
>> +	unsigned long flags;
>> +
>>   	ENTER();
>>   
>>   	if (atomic_dec_and_test(&ffs->opened)) {
>>   		if (ffs->no_disconnect) {
>>   			ffs->state = FFS_DEACTIVATED;
>> -			if (ffs->epfiles) {
>> -				ffs_epfiles_destroy(ffs->epfiles,
>> -						   ffs->eps_count);
>> -				ffs->epfiles = NULL;
>> -			}
>> +			spin_lock_irqsave(&ffs->eps_lock, flags);
>> +			epfiles = ffs->epfiles;
>> +			ffs->epfiles = NULL;
>> +			spin_unlock_irqrestore(&ffs->eps_lock,
>> +							flags);
>> +
>> +			if (epfiles)
>> +				ffs_epfiles_destroy(epfiles,
>> +						 ffs->eps_count);
> You are accessing epfiles outside of the lock.  How is that ok?
There are 2 parties that are trying to free the ffs->epfiles. We use the 
lock to serialize which party gets to free the ffs->epfiles. Where we 
are making it NULL we protected that, The freeing part doesn't need to 
be inside spinlock because now it is epfiles(local_ variable) with which 
we are trying to operate now. We removed the ffs->epfiles as the common 
structure to operate and made the local variable epfiles to do the job.
>
>> +
>>   			if (ffs->setup_state == FFS_SETUP_PENDING)
>>   				__ffs_ep0_stall(ffs);
>>   		} else {
>> @@ -1767,14 +1775,27 @@ static struct ffs_data *ffs_data_new(const char *dev_name)
>>   
>>   static void ffs_data_clear(struct ffs_data *ffs)
>>   {
>> +	struct ffs_epfile *epfiles;
>> +	unsigned long flags;
>> +
>>   	ENTER();
>>   
>>   	ffs_closed(ffs);
>>   
>>   	BUG_ON(ffs->gadget);
>>   
>> -	if (ffs->epfiles)
>> -		ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count);
>> +	spin_lock_irqsave(&ffs->eps_lock, flags);
>> +	epfiles = ffs->epfiles;
>> +	ffs->epfiles = NULL;
>> +	spin_unlock_irqrestore(&ffs->eps_lock, flags);
>> +
>> +	/*
>> +	 * potential race possible between ffs_func_eps_disable
>> +	 * & ffs_epfile_release therefore maintaining a local
>> +	 * copy of epfile will save us from use-after-free.
>> +	 */
>> +	if (epfiles)
>> +		ffs_epfiles_destroy(epfiles, ffs->eps_count);
> How will it save you from a use-after-free as you never increased a
> reference count on the pointer here?
 From the above we already serialized the one who gets to free it. So 
whoever gets to access will mark it NULL so the other function trying to 
access ffs->epfiles backs off. I think reference count is not applicable 
here.
>
> And don't write code with new races.  Or are you trying to say this
> fixes a race?
Yes this is for an existing race which we encountered.
>
> Just saving off a pointer feels wrong here.  What happens if the lock is
> properly grabbed, then freed, right after you unlock?  You aren't really
> protecting this properly in a normal way that reference counts and locks
> work from what I can tell, only trying to reduce the race window but not
> fixing it.
This I Doubt because the reference count might not be decremented at 
anycase.
>
> thanks,
>
> greg k-h

Re: [PATCH v6] usb: f_fs: Fix use-after-free for epfile

[Greg Kroah-Hartman]

On Mon, Dec 20, 2021 at 07:16:14PM +0530, Udipto Goswami wrote:
> Hi Greg,
> 
> Apologies for the delay, please find the following comments as per my
> understanding inline.
> 
> On 13-12-2021 07:38 pm, Greg Kroah-Hartman wrote:
> > On Tue, Dec 07, 2021 at 11:37:54AM +0530, Udipto Goswami wrote:
> > > Consider a case where ffs_func_eps_disable is called from
> > > ffs_func_disable as part of composition switch and at the
> > > same time ffs_epfile_release get called from userspace.
> > > ffs_epfile_release will free up the read buffer and call
> > > ffs_data_closed which in turn destroys ffs->epfiles and
> > > mark it as NULL. While this was happening the driver has
> > > already initialized the local epfile in ffs_func_eps_disable
> > > which is now freed and waiting to acquire the spinlock. Once
> > > spinlock is acquired the driver proceeds with the stale value
> > > of epfile and tries to free the already freed read buffer
> > > causing use-after-free.
> > > 
> > > Following is the illustration of the race:
> > > 
> > >        CPU1                                  CPU2
> > > 
> > >     ffs_func_eps_disable
> > >     epfiles (local copy)
> > > 					ffs_epfile_release
> > > 					ffs_data_closed
> > > 					if (last file closed)
> > > 					ffs_data_reset
> > > 					ffs_data_clear
> > > 					ffs_epfiles_destroy
> > > spin_lock
> > > dereference epfiles
> > > 
> > > Fix this races by taking epfiles local copy & assigning it under
> > > spinlock and if epfiles(local) is null then update it in ffs->epfiles
> > > then finally destroy it.
> Fixes: a9e6f83c2df (usb: gadget: f_fs: stop sleeping in
> ffs_func_eps_disable)
> > > Reviewed-by: John Keeping <john@metanate.com>
> > > Signed-off-by: Pratham Pratap <quic_ppratap@quicinc.com>
> > > Co-developed-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> > > Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> > > ---
> > > v6: Addressing minor code formatting.
> > > 
> > >   drivers/usb/gadget/function/f_fs.c | 44 +++++++++++++++++++++++++++++---------
> > >   1 file changed, 34 insertions(+), 10 deletions(-)
> > What commit does this fix?
> > 
> > Does this need to go to stable kernel releases?
> > 
> > thanks,
> > 
> > greg k-h
> > 
> This is fixing Commit:  a9e6f83c2df (usb: gadget: f_fs: stop sleeping in
> ffs_func_eps_disable)
> 
> Yes this fix is needed for >=5.4 Kernel releases.

Please put that in the patch when you resend it.

> > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > > index 3c584da..6f23a66 100644
> > > --- a/drivers/usb/gadget/function/f_fs.c
> > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > @@ -1711,16 +1711,24 @@ static void ffs_data_put(struct ffs_data *ffs)
> > >   static void ffs_data_closed(struct ffs_data *ffs)
> > >   {
> > > +	struct ffs_epfile *epfiles;
> > > +	unsigned long flags;
> > > +
> > >   	ENTER();
> > >   	if (atomic_dec_and_test(&ffs->opened)) {
> > >   		if (ffs->no_disconnect) {
> > >   			ffs->state = FFS_DEACTIVATED;
> > > -			if (ffs->epfiles) {
> > > -				ffs_epfiles_destroy(ffs->epfiles,
> > > -						   ffs->eps_count);
> > > -				ffs->epfiles = NULL;
> > > -			}
> > > +			spin_lock_irqsave(&ffs->eps_lock, flags);
> > > +			epfiles = ffs->epfiles;
> > > +			ffs->epfiles = NULL;
> > > +			spin_unlock_irqrestore(&ffs->eps_lock,
> > > +							flags);
> > > +
> > > +			if (epfiles)
> > > +				ffs_epfiles_destroy(epfiles,
> > > +						 ffs->eps_count);
> > You are accessing epfiles outside of the lock.  How is that ok?
> There are 2 parties that are trying to free the ffs->epfiles. We use the
> lock to serialize which party gets to free the ffs->epfiles. Where we are
> making it NULL we protected that, The freeing part doesn't need to be inside
> spinlock because now it is epfiles(local_ variable) with which we are trying
> to operate now. We removed the ffs->epfiles as the common structure to
> operate and made the local variable epfiles to do the job.

Why is the lock used only for that one field and not all other fields in
that structure?  What about the no_disconnect access above these lines?
The setup_state access below them?  Why does only one pointer deserve
this locking and not all of them?

thanks,

greg k-h

Re: [PATCH v6] usb: f_fs: Fix use-after-free for epfile

[Udipto Goswami]

Hi Greg,

On 21-12-2021 01:16 pm, Greg Kroah-Hartman wrote:
> On Mon, Dec 20, 2021 at 07:16:14PM +0530, Udipto Goswami wrote:
>> Hi Greg,
>>
>> Apologies for the delay, please find the following comments as per my
>> understanding inline.
>>
>> On 13-12-2021 07:38 pm, Greg Kroah-Hartman wrote:
>>> On Tue, Dec 07, 2021 at 11:37:54AM +0530, Udipto Goswami wrote:
>>>> Consider a case where ffs_func_eps_disable is called from
>>>> ffs_func_disable as part of composition switch and at the
>>>> same time ffs_epfile_release get called from userspace.
>>>> ffs_epfile_release will free up the read buffer and call
>>>> ffs_data_closed which in turn destroys ffs->epfiles and
>>>> mark it as NULL. While this was happening the driver has
>>>> already initialized the local epfile in ffs_func_eps_disable
>>>> which is now freed and waiting to acquire the spinlock. Once
>>>> spinlock is acquired the driver proceeds with the stale value
>>>> of epfile and tries to free the already freed read buffer
>>>> causing use-after-free.
>>>>
>>>> Following is the illustration of the race:
>>>>
>>>>         CPU1                                  CPU2
>>>>
>>>>      ffs_func_eps_disable
>>>>      epfiles (local copy)
>>>> 					ffs_epfile_release
>>>> 					ffs_data_closed
>>>> 					if (last file closed)
>>>> 					ffs_data_reset
>>>> 					ffs_data_clear
>>>> 					ffs_epfiles_destroy
>>>> spin_lock
>>>> dereference epfiles
>>>>
>>>> Fix this races by taking epfiles local copy & assigning it under
>>>> spinlock and if epfiles(local) is null then update it in ffs->epfiles
>>>> then finally destroy it.
>> Fixes: a9e6f83c2df (usb: gadget: f_fs: stop sleeping in
>> ffs_func_eps_disable)
>>>> Reviewed-by: John Keeping <john@metanate.com>
>>>> Signed-off-by: Pratham Pratap <quic_ppratap@quicinc.com>
>>>> Co-developed-by: Udipto Goswami <quic_ugoswami@quicinc.com>
>>>> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
>>>> ---
>>>> v6: Addressing minor code formatting.
>>>>
>>>>    drivers/usb/gadget/function/f_fs.c | 44 +++++++++++++++++++++++++++++---------
>>>>    1 file changed, 34 insertions(+), 10 deletions(-)
>>> What commit does this fix?
>>>
>>> Does this need to go to stable kernel releases?
>>>
>>> thanks,
>>>
>>> greg k-h
>>>
>> This is fixing Commit:  a9e6f83c2df (usb: gadget: f_fs: stop sleeping in
>> ffs_func_eps_disable)
>>
>> Yes this fix is needed for >=5.4 Kernel releases.
> Please put that in the patch when you resend it.
>
>>>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>>>> index 3c584da..6f23a66 100644
>>>> --- a/drivers/usb/gadget/function/f_fs.c
>>>> +++ b/drivers/usb/gadget/function/f_fs.c
>>>> @@ -1711,16 +1711,24 @@ static void ffs_data_put(struct ffs_data *ffs)
>>>>    static void ffs_data_closed(struct ffs_data *ffs)
>>>>    {
>>>> +	struct ffs_epfile *epfiles;
>>>> +	unsigned long flags;
>>>> +
>>>>    	ENTER();
>>>>    	if (atomic_dec_and_test(&ffs->opened)) {
>>>>    		if (ffs->no_disconnect) {
>>>>    			ffs->state = FFS_DEACTIVATED;
>>>> -			if (ffs->epfiles) {
>>>> -				ffs_epfiles_destroy(ffs->epfiles,
>>>> -						   ffs->eps_count);
>>>> -				ffs->epfiles = NULL;
>>>> -			}
>>>> +			spin_lock_irqsave(&ffs->eps_lock, flags);
>>>> +			epfiles = ffs->epfiles;
>>>> +			ffs->epfiles = NULL;
>>>> +			spin_unlock_irqrestore(&ffs->eps_lock,
>>>> +							flags);
>>>> +
>>>> +			if (epfiles)
>>>> +				ffs_epfiles_destroy(epfiles,
>>>> +						 ffs->eps_count);
>>> You are accessing epfiles outside of the lock.  How is that ok?
>> There are 2 parties that are trying to free the ffs->epfiles. We use the
>> lock to serialize which party gets to free the ffs->epfiles. Where we are
>> making it NULL we protected that, The freeing part doesn't need to be inside
>> spinlock because now it is epfiles(local_ variable) with which we are trying
>> to operate now. We removed the ffs->epfiles as the common structure to
>> operate and made the local variable epfiles to do the job.
> Why is the lock used only for that one field and not all other fields in
> that structure?  What about the no_disconnect access above these lines?
> The setup_state access below them?  Why does only one pointer deserve
> this locking and not all of them?
I believe what you are trying to reference for protecting the ffs_data 
structure is a little tricky. I understand that the entire ffs_data 
isn't itself protected between concurrent cases but it will be probably 
a bigger redesign which is out of scope of this patch.

Here we were trying only to protect the epfiles structure, which we get 
from ffs_epfiles_create() ffs_func_eps_enable() is where we assign it 
and ffs_data_reset() is where we are clearing it.

Maybe we can broaden the scope of this patch and try to protect the 
epfiles in all the places we are using?
something like this?

@@ -1811,7 +1811,6 @@ static void ffs_data_reset(struct ffs_data *ffs)

          ffs_data_clear(ffs);

-       ffs->epfiles = NULL;
          ffs->raw_descs_data = NULL;
          ffs->raw_descs = NULL;
          ffs->raw_strings = NULL;
@@ -1899,6 +1898,7 @@ static int ffs_epfiles_create(struct ffs_data *ffs)
          if (!epfiles)
                  return -ENOMEM;

+       spin_lock_irqsave(&ffs->eps_lock, flags);
          epfile = epfiles;
          for (i = 1; i <= count; ++i, ++epfile) {
                  epfile->ffs = ffs;
@@ -1912,11 +1912,13 @@ static int ffs_epfiles_create(struct ffs_data 
*ffs) &ffs_epfile_operations);
                  if (!epfile->dentry) {
                          ffs_epfiles_destroy(epfiles, i - 1);
+ spin_unlock_irqrestore(&ffs->eps_lock, flags);

should we be adding epfiles = NULL?

                          return -ENOMEM;
                  }
          }

          ffs->epfiles = epfiles;
+       spin_unlock_irqrestore(&ffs->eps_lock, flags);
          return 0;
   }

@@ -1966,14 +1968,18 @@ static void ffs_func_eps_disable(struct 
ffs_function *func)

   static int ffs_func_eps_enable(struct ffs_function *func)
   {
-       struct ffs_data *ffs      = func->ffs;
-       struct ffs_ep *ep         = func->eps;
-       struct ffs_epfile *epfile = ffs->epfiles;
-       unsigned count            = ffs->eps_count;
+       struct ffs_data *ffs;
+       struct ffs_ep *ep;
+       struct ffs_epfile *epfile;
+       unsigned count;
          unsigned long flags;
          int ret = 0;

          spin_lock_irqsave(&func->ffs->eps_lock, flags);
+       ffs = func->ffs;
+       ep = func->eps;
+       epfiles = ffs->epfiles;
+       count = ffs->eps_count;
          while(count--) {
                  ep->ep->driver_data = ep;

This is the intention actually to protect the epfiles. This additional 
snippet will cover other EP related structures, and concurrent accesses 
which i have mentioned in the illustration of the race.

We are more interested in protecting epfiles is because we have multiple 
report from our customers indicating epfiles is accessed after free and 
it is fixed with this patch.
We can work on the ffs_data protection seperately if required.


Thanks,

-Udipto