[PATCH] thunderbolt: Fix a double put in tb_cfg_read_raw

[PATCH] thunderbolt: Fix a double put in tb_cfg_read_raw

[Lv Yunlong]

In tb_cfg_read_raw, req is allocated by tb_cfg_request_alloc()
with an initial reference. Before calling tb_cfg_request_sync(),
there is no refcount inc operation. tb_cfg_request_sync()
calls tb_cfg_request(..,req,..) and if the callee failed,
the initial reference of req is dropped and req is freed.

Later in tb_cfg_read_raw before the err check,
tb_cfg_request_put(req) is called again. It may cause error
in race.

My patch puts tb_cfg_request_put(req) after the err check
finished to avoid unexpected result.

Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
---
 drivers/thunderbolt/ctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c
index f1aeaff9f368..bb60269c89ab 100644
--- a/drivers/thunderbolt/ctl.c
+++ b/drivers/thunderbolt/ctl.c
@@ -890,11 +890,11 @@ struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl *ctl, void *buffer,
 
 		res = tb_cfg_request_sync(ctl, req, timeout_msec);
 
-		tb_cfg_request_put(req);
-
 		if (res.err != -ETIMEDOUT)
 			break;
 
+		tb_cfg_request_put(req);
+
 		/* Wait a bit (arbitrary time) until we send a retry */
 		usleep_range(10, 100);
 	}
-- 
2.25.1

Re: [PATCH] thunderbolt: Fix a double put in tb_cfg_read_raw

[Mika Westerberg]

Hi,

On Mon, Mar 22, 2021 at 08:15:12PM -0700, Lv Yunlong wrote:
> In tb_cfg_read_raw, req is allocated by tb_cfg_request_alloc()
> with an initial reference. Before calling tb_cfg_request_sync(),
> there is no refcount inc operation. tb_cfg_request_sync()
> calls tb_cfg_request(..,req,..) and if the callee failed,
> the initial reference of req is dropped and req is freed.
> 
> Later in tb_cfg_read_raw before the err check,
> tb_cfg_request_put(req) is called again. It may cause error
> in race.

Hmm, tb_cfg_request() does tb_cfg_request_get() too and in case of error
it does tb_cfg_request_put(). So the refcount should be fine. What am I
missing?

> 
> My patch puts tb_cfg_request_put(req) after the err check
> finished to avoid unexpected result.
> 
> Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
> ---
>  drivers/thunderbolt/ctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c
> index f1aeaff9f368..bb60269c89ab 100644
> --- a/drivers/thunderbolt/ctl.c
> +++ b/drivers/thunderbolt/ctl.c
> @@ -890,11 +890,11 @@ struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl *ctl, void *buffer,
>  
>  		res = tb_cfg_request_sync(ctl, req, timeout_msec);
>  
> -		tb_cfg_request_put(req);
> -
>  		if (res.err != -ETIMEDOUT)
>  			break;
>  
> +		tb_cfg_request_put(req);
> +
>  		/* Wait a bit (arbitrary time) until we send a retry */
>  		usleep_range(10, 100);
>  	}
> -- 
> 2.25.1
> 

Re: Re: [PATCH] thunderbolt: Fix a double put in tb_cfg_read_raw

[lyl2019]

> -----原始邮件-----
> 发件人: "Mika Westerberg" <mika.westerberg@linux.intel.com>
> 发送时间: 2021-03-23 22:06:47 (星期二)
> 收件人: "Lv Yunlong" <lyl2019@mail.ustc.edu.cn>
> 抄送: andreas.noever@gmail.com, michael.jamet@intel.com, YehezkelShB@gmail.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
> 主题: Re: [PATCH] thunderbolt: Fix a double put in tb_cfg_read_raw
> 
> Hi,
> 
> On Mon, Mar 22, 2021 at 08:15:12PM -0700, Lv Yunlong wrote:
> > In tb_cfg_read_raw, req is allocated by tb_cfg_request_alloc()
> > with an initial reference. Before calling tb_cfg_request_sync(),
> > there is no refcount inc operation. tb_cfg_request_sync()
> > calls tb_cfg_request(..,req,..) and if the callee failed,
> > the initial reference of req is dropped and req is freed.
> > 
> > Later in tb_cfg_read_raw before the err check,
> > tb_cfg_request_put(req) is called again. It may cause error
> > in race.
> 
> Hmm, tb_cfg_request() does tb_cfg_request_get() too and in case of error
> it does tb_cfg_request_put(). So the refcount should be fine. What am I
> missing?
> 
> > 
> > My patch puts tb_cfg_request_put(req) after the err check
> > finished to avoid unexpected result.
> > 
> > Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
> > ---
> >  drivers/thunderbolt/ctl.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c
> > index f1aeaff9f368..bb60269c89ab 100644
> > --- a/drivers/thunderbolt/ctl.c
> > +++ b/drivers/thunderbolt/ctl.c
> > @@ -890,11 +890,11 @@ struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl *ctl, void *buffer,
> >  
> >  		res = tb_cfg_request_sync(ctl, req, timeout_msec);
> >  
> > -		tb_cfg_request_put(req);
> > -
> >  		if (res.err != -ETIMEDOUT)
> >  			break;
> >  
> > +		tb_cfg_request_put(req);
> > +
> >  		/* Wait a bit (arbitrary time) until we send a retry */
> >  		usleep_range(10, 100);
> >  	}
> > -- 
> > 2.25.1
> > 

I'm very sorry, i was ashamed that i had missed the tb_cfg_request_get() in tb_cfg_request().

Thanks.

Re: Re: [PATCH] thunderbolt: Fix a double put in tb_cfg_read_raw

[Mika Westerberg]

On Tue, Mar 23, 2021 at 10:30:16PM +0800, lyl2019@mail.ustc.edu.cn wrote:
> 
> 
> 
> > -----原始邮件-----
> > 发件人: "Mika Westerberg" <mika.westerberg@linux.intel.com>
> > 发送时间: 2021-03-23 22:06:47 (星期二)
> > 收件人: "Lv Yunlong" <lyl2019@mail.ustc.edu.cn>
> > 抄送: andreas.noever@gmail.com, michael.jamet@intel.com, YehezkelShB@gmail.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
> > 主题: Re: [PATCH] thunderbolt: Fix a double put in tb_cfg_read_raw
> > 
> > Hi,
> > 
> > On Mon, Mar 22, 2021 at 08:15:12PM -0700, Lv Yunlong wrote:
> > > In tb_cfg_read_raw, req is allocated by tb_cfg_request_alloc()
> > > with an initial reference. Before calling tb_cfg_request_sync(),
> > > there is no refcount inc operation. tb_cfg_request_sync()
> > > calls tb_cfg_request(..,req,..) and if the callee failed,
> > > the initial reference of req is dropped and req is freed.
> > > 
> > > Later in tb_cfg_read_raw before the err check,
> > > tb_cfg_request_put(req) is called again. It may cause error
> > > in race.
> > 
> > Hmm, tb_cfg_request() does tb_cfg_request_get() too and in case of error
> > it does tb_cfg_request_put(). So the refcount should be fine. What am I
> > missing?
> > 
> > > 
> > > My patch puts tb_cfg_request_put(req) after the err check
> > > finished to avoid unexpected result.
> > > 
> > > Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
> > > ---
> > >  drivers/thunderbolt/ctl.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c
> > > index f1aeaff9f368..bb60269c89ab 100644
> > > --- a/drivers/thunderbolt/ctl.c
> > > +++ b/drivers/thunderbolt/ctl.c
> > > @@ -890,11 +890,11 @@ struct tb_cfg_result tb_cfg_read_raw(struct tb_ctl *ctl, void *buffer,
> > >  
> > >  		res = tb_cfg_request_sync(ctl, req, timeout_msec);
> > >  
> > > -		tb_cfg_request_put(req);
> > > -
> > >  		if (res.err != -ETIMEDOUT)
> > >  			break;
> > >  
> > > +		tb_cfg_request_put(req);
> > > +
> > >  		/* Wait a bit (arbitrary time) until we send a retry */
> > >  		usleep_range(10, 100);
> > >  	}
> > > -- 
> > > 2.25.1
> > > 
> 
> I'm very sorry, i was ashamed that i had missed the tb_cfg_request_get() in tb_cfg_request().

It happens, no worries :)