RE: [PATCH] firmware loader: Fix the race FW_STATUS_DONE is followed by class_timeout

2012-11-07 Thread Liu, Chuansheng 


> -Original Message-
> From: Ming Lei [mailto:ming@canonical.com]
> Sent: Wednesday, November 07, 2012 6:40 PM
> To: Liu, Chuansheng
> Cc: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] firmware loader: Fix the race FW_STATUS_DONE is
> followed by class_timeout
> 
> On Thu, Nov 8, 2012 at 12:45 AM, Chuansheng Liu
>  wrote:
> >
> > There is a race condition as below when calling request_firmware():
> >
> > CPU1CPU2
> > write 0 > loading
> > mutex_lock(_lock);
> > ...
> > set_bit FW_STATUS_DONE  class_timeout is coming
> > set_bit
> FW_STATUS_ABORT
> > complete_all 
> > ...
> > mutex_unlock(_lock)
> 
> Good catch, but your fix isn't correct.
> 
> >
> > In this time, the bit FW_STATUS_DONE and FW_STATUS_ABORT are set,
> > and request_firmware() will return failure due to condition in
> > _request_firmware_load():
> >
> > if (!buf->size || test_bit(FW_STATUS_ABORT, >status))
> > retval = -ENOENT;
> >
> > But from the above scenerio, it should be a succcessful requesting.
> > So we need judge if the FW_STATUS_DONE is set before calling abort
> > in timeout function firmware_class_timeout().
> >
> > Signed-off-by: liu chuansheng 
> > ---
> >  drivers/base/firmware_class.c |7 +++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 8945f4e..35fffd8 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -671,6 +671,13 @@ static void firmware_class_timeout(u_long data)
> >  {
> > struct firmware_priv *fw_priv = (struct firmware_priv *) data;
> >
> > +   mutex_lock(_lock);
> 
> mutex can't be used in interrupt context, so one candidate fix is
> to convert the timeout timer into delayed work, and hold the
> 'fw_lock' during the work function.
> 
> > +   if (test_bit(FW_STATUS_DONE, &(fw_priv->buf->status))) {
> > +   mutex_unlock(_lock);
> > +   return;
> > +   }
> > +   mutex_unlock(_lock);
> > +
> > fw_load_abort(fw_priv);
> >  }
> 
> Also the lock of 'fw_lock' should be held when setting 'FW_STATUS_DONE'
> in _request_firmware_load().
> 
> Could you send out a new patch to fix the race?
Thanks your review and proposal, has sent the patch V2 to be reviewed.
> 
> Thanks,
> --
> Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] firmware loader: Fix the race FW_STATUS_DONE is followed by class_timeout

2012-11-07 Thread Ming Lei 
On Thu, Nov 8, 2012 at 12:45 AM, Chuansheng Liu
 wrote:
>
> There is a race condition as below when calling request_firmware():
>
> CPU1CPU2
> write 0 > loading
> mutex_lock(_lock);
> ...
> set_bit FW_STATUS_DONE  class_timeout is coming
> set_bit FW_STATUS_ABORT
> complete_all 
> ...
> mutex_unlock(_lock)

Good catch, but your fix isn't correct.

>
> In this time, the bit FW_STATUS_DONE and FW_STATUS_ABORT are set,
> and request_firmware() will return failure due to condition in
> _request_firmware_load():
>
> if (!buf->size || test_bit(FW_STATUS_ABORT, >status))
> retval = -ENOENT;
>
> But from the above scenerio, it should be a succcessful requesting.
> So we need judge if the FW_STATUS_DONE is set before calling abort
> in timeout function firmware_class_timeout().
>
> Signed-off-by: liu chuansheng 
> ---
>  drivers/base/firmware_class.c |7 +++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8945f4e..35fffd8 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -671,6 +671,13 @@ static void firmware_class_timeout(u_long data)
>  {
> struct firmware_priv *fw_priv = (struct firmware_priv *) data;
>
> +   mutex_lock(_lock);

mutex can't be used in interrupt context, so one candidate fix is
to convert the timeout timer into delayed work, and hold the
'fw_lock' during the work function.

> +   if (test_bit(FW_STATUS_DONE, &(fw_priv->buf->status))) {
> +   mutex_unlock(_lock);
> +   return;
> +   }
> +   mutex_unlock(_lock);
> +
> fw_load_abort(fw_priv);
>  }

Also the lock of 'fw_lock' should be held when setting 'FW_STATUS_DONE'
in _request_firmware_load().

Could you send out a new patch to fix the race?

Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] firmware loader: Fix the race FW_STATUS_DONE is followed by class_timeout

2012-11-07 Thread Ming Lei 
On Thu, Nov 8, 2012 at 12:45 AM, Chuansheng Liu
chuansheng@intel.com wrote:

 There is a race condition as below when calling request_firmware():

 CPU1CPU2
 write 0  loading
 mutex_lock(fw_lock);
 ...
 set_bit FW_STATUS_DONE  class_timeout is coming
 set_bit FW_STATUS_ABORT
 complete_all completion
 ...
 mutex_unlock(fw_lock)

Good catch, but your fix isn't correct.


 In this time, the bit FW_STATUS_DONE and FW_STATUS_ABORT are set,
 and request_firmware() will return failure due to condition in
 _request_firmware_load():

 if (!buf-size || test_bit(FW_STATUS_ABORT, buf-status))
 retval = -ENOENT;

 But from the above scenerio, it should be a succcessful requesting.
 So we need judge if the FW_STATUS_DONE is set before calling abort
 in timeout function firmware_class_timeout().

 Signed-off-by: liu chuansheng chuansheng@intel.com
 ---
  drivers/base/firmware_class.c |7 +++
  1 files changed, 7 insertions(+), 0 deletions(-)

 diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
 index 8945f4e..35fffd8 100644
 --- a/drivers/base/firmware_class.c
 +++ b/drivers/base/firmware_class.c
 @@ -671,6 +671,13 @@ static void firmware_class_timeout(u_long data)
  {
 struct firmware_priv *fw_priv = (struct firmware_priv *) data;

 +   mutex_lock(fw_lock);

mutex can't be used in interrupt context, so one candidate fix is
to convert the timeout timer into delayed work, and hold the
'fw_lock' during the work function.

 +   if (test_bit(FW_STATUS_DONE, (fw_priv-buf-status))) {
 +   mutex_unlock(fw_lock);
 +   return;
 +   }
 +   mutex_unlock(fw_lock);
 +
 fw_load_abort(fw_priv);
  }

Also the lock of 'fw_lock' should be held when setting 'FW_STATUS_DONE'
in _request_firmware_load().

Could you send out a new patch to fix the race?

Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] firmware loader: Fix the race FW_STATUS_DONE is followed by class_timeout

2012-11-07 Thread Liu, Chuansheng 


 -Original Message-
 From: Ming Lei [mailto:ming@canonical.com]
 Sent: Wednesday, November 07, 2012 6:40 PM
 To: Liu, Chuansheng
 Cc: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org
 Subject: Re: [PATCH] firmware loader: Fix the race FW_STATUS_DONE is
 followed by class_timeout
 
 On Thu, Nov 8, 2012 at 12:45 AM, Chuansheng Liu
 chuansheng@intel.com wrote:
 
  There is a race condition as below when calling request_firmware():
 
  CPU1CPU2
  write 0  loading
  mutex_lock(fw_lock);
  ...
  set_bit FW_STATUS_DONE  class_timeout is coming
  set_bit
 FW_STATUS_ABORT
  complete_all completion
  ...
  mutex_unlock(fw_lock)
 
 Good catch, but your fix isn't correct.
 
 
  In this time, the bit FW_STATUS_DONE and FW_STATUS_ABORT are set,
  and request_firmware() will return failure due to condition in
  _request_firmware_load():
 
  if (!buf-size || test_bit(FW_STATUS_ABORT, buf-status))
  retval = -ENOENT;
 
  But from the above scenerio, it should be a succcessful requesting.
  So we need judge if the FW_STATUS_DONE is set before calling abort
  in timeout function firmware_class_timeout().
 
  Signed-off-by: liu chuansheng chuansheng@intel.com
  ---
   drivers/base/firmware_class.c |7 +++
   1 files changed, 7 insertions(+), 0 deletions(-)
 
  diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
  index 8945f4e..35fffd8 100644
  --- a/drivers/base/firmware_class.c
  +++ b/drivers/base/firmware_class.c
  @@ -671,6 +671,13 @@ static void firmware_class_timeout(u_long data)
   {
  struct firmware_priv *fw_priv = (struct firmware_priv *) data;
 
  +   mutex_lock(fw_lock);
 
 mutex can't be used in interrupt context, so one candidate fix is
 to convert the timeout timer into delayed work, and hold the
 'fw_lock' during the work function.
 
  +   if (test_bit(FW_STATUS_DONE, (fw_priv-buf-status))) {
  +   mutex_unlock(fw_lock);
  +   return;
  +   }
  +   mutex_unlock(fw_lock);
  +
  fw_load_abort(fw_priv);
   }
 
 Also the lock of 'fw_lock' should be held when setting 'FW_STATUS_DONE'
 in _request_firmware_load().
 
 Could you send out a new patch to fix the race?
Thanks your review and proposal, has sent the patch V2 to be reviewed.
 
 Thanks,
 --
 Ming Lei
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/