* [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add()
@ 2021-03-29 6:07 Dan Carpenter
2021-03-29 6:08 ` [PATCH 2/2] thunderbolt: Fix off by one in tb_port_find_retimer() Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-03-29 6:07 UTC (permalink / raw)
To: Andreas Noever, Kranthi Kuntala
Cc: Michael Jamet, Mika Westerberg, Yehezkel Bernat, linux-usb,
linux-kernel, kernel-janitors, Jason Gunthorpe
After the device_register() succeeds, then the correct way to clean up
is to call device_unregister(). The unregister calls both device_del()
and device_put(). Since this code was only device_del() it results in
a memory leak.
Fixes: dacb12877d92 ("thunderbolt: Add support for on-board retimers")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is from a new static checker warning. Not tested. With new
warnings it's also possible that I have misunderstood something
fundamental so review carefully etc.
drivers/thunderbolt/retimer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c
index 620bcf586ee2..7a5d61604c8b 100644
--- a/drivers/thunderbolt/retimer.c
+++ b/drivers/thunderbolt/retimer.c
@@ -347,7 +347,7 @@ static int tb_retimer_add(struct tb_port *port, u8 index, u32 auth_status)
ret = tb_retimer_nvm_add(rt);
if (ret) {
dev_err(&rt->dev, "failed to add NVM devices: %d\n", ret);
- device_del(&rt->dev);
+ device_unregister(&rt->dev);
return ret;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] thunderbolt: Fix off by one in tb_port_find_retimer()
2021-03-29 6:07 [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add() Dan Carpenter
@ 2021-03-29 6:08 ` Dan Carpenter
2021-03-29 13:02 ` [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add() Jason Gunthorpe
2021-03-30 10:41 ` Mika Westerberg
2 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-03-29 6:08 UTC (permalink / raw)
To: Andreas Noever, Kranthi Kuntala
Cc: Michael Jamet, Mika Westerberg, Yehezkel Bernat, linux-usb,
linux-kernel, kernel-janitors
This array uses 1-based indexing so it corrupts memory one element
beyond of the array. Fix it by making the array one element larger.
Fixes: dacb12877d92 ("thunderbolt: Add support for on-board retimers")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/thunderbolt/retimer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c
index 7a5d61604c8b..c44fad2b9fbb 100644
--- a/drivers/thunderbolt/retimer.c
+++ b/drivers/thunderbolt/retimer.c
@@ -406,7 +406,7 @@ static struct tb_retimer *tb_port_find_retimer(struct tb_port *port, u8 index)
*/
int tb_retimer_scan(struct tb_port *port)
{
- u32 status[TB_MAX_RETIMER_INDEX] = {};
+ u32 status[TB_MAX_RETIMER_INDEX + 1] = {};
int ret, i, last_idx = 0;
if (!port->cap_usb4)
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add()
2021-03-29 6:07 [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add() Dan Carpenter
2021-03-29 6:08 ` [PATCH 2/2] thunderbolt: Fix off by one in tb_port_find_retimer() Dan Carpenter
@ 2021-03-29 13:02 ` Jason Gunthorpe
2021-03-29 14:43 ` Mika Westerberg
2021-03-30 10:41 ` Mika Westerberg
2 siblings, 1 reply; 7+ messages in thread
From: Jason Gunthorpe @ 2021-03-29 13:02 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andreas Noever, Kranthi Kuntala, Michael Jamet, Mika Westerberg,
Yehezkel Bernat, linux-usb, linux-kernel, kernel-janitors
On Mon, Mar 29, 2021 at 09:07:18AM +0300, Dan Carpenter wrote:
> After the device_register() succeeds, then the correct way to clean up
> is to call device_unregister(). The unregister calls both device_del()
> and device_put(). Since this code was only device_del() it results in
> a memory leak.
>
> Fixes: dacb12877d92 ("thunderbolt: Add support for on-board retimers")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This is from a new static checker warning. Not tested. With new
> warnings it's also possible that I have misunderstood something
> fundamental so review carefully etc.
It looks OK to me
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
This also highlights the code has an ordering issue too, it calls
device_register() then goes to do tb_retimer_nvm_add() however
device_register() makes sysfs attributes visible before the rt->nvm is
initialized and this:
static ssize_t nvm_authenticate_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)
{
if (!rt->nvm) {
Isn't strong enough to close the potential racing. The nvm should be
setup before device_register and all the above tests in the sysfs
deleted so we can rely on the CPU barriers built into
device_register() for correctness.
[which is a general tip, be very suspicious if device_register() is
being error unwound]
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add()
2021-03-29 13:02 ` [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add() Jason Gunthorpe
@ 2021-03-29 14:43 ` Mika Westerberg
2021-03-29 14:54 ` Jason Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Mika Westerberg @ 2021-03-29 14:43 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Dan Carpenter, Andreas Noever, Kranthi Kuntala, Michael Jamet,
Yehezkel Bernat, linux-usb, linux-kernel, kernel-janitors
Hi,
On Mon, Mar 29, 2021 at 10:02:20AM -0300, Jason Gunthorpe wrote:
> On Mon, Mar 29, 2021 at 09:07:18AM +0300, Dan Carpenter wrote:
> > After the device_register() succeeds, then the correct way to clean up
> > is to call device_unregister(). The unregister calls both device_del()
> > and device_put(). Since this code was only device_del() it results in
> > a memory leak.
> >
> > Fixes: dacb12877d92 ("thunderbolt: Add support for on-board retimers")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > This is from a new static checker warning. Not tested. With new
> > warnings it's also possible that I have misunderstood something
> > fundamental so review carefully etc.
>
> It looks OK to me
I agree too.
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Thanks for the review!
> This also highlights the code has an ordering issue too, it calls
> device_register() then goes to do tb_retimer_nvm_add() however
> device_register() makes sysfs attributes visible before the rt->nvm is
> initialized and this:
>
> static ssize_t nvm_authenticate_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t count)
> {
> if (!rt->nvm) {
>
> Isn't strong enough to close the potential racing. The nvm should be
> setup before device_register and all the above tests in the sysfs
> deleted so we can rely on the CPU barriers built into
> device_register() for correctness.
>
> [which is a general tip, be very suspicious if device_register() is
> being error unwound]
The nvm is a separate (physical Linux) device that gets added under this
one. It cannot be added before AFAICT.
The code you refer actually looks like this:
static ssize_t nvm_authenticate_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)
{
...
if (!mutex_trylock(&rt->tb->lock)) {
ret = restart_syscall();
goto exit_rpm;
}
if (!rt->nvm) {
ret = -EAGAIN;
goto exit_unlock;
}
Idea here is that if the NVMem (nvm) is not yet registered the attribute is
there but we return -EAGAIN to the userspace.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add()
2021-03-29 14:43 ` Mika Westerberg
@ 2021-03-29 14:54 ` Jason Gunthorpe
2021-03-29 15:06 ` Mika Westerberg
0 siblings, 1 reply; 7+ messages in thread
From: Jason Gunthorpe @ 2021-03-29 14:54 UTC (permalink / raw)
To: Mika Westerberg
Cc: Dan Carpenter, Andreas Noever, Kranthi Kuntala, Michael Jamet,
Yehezkel Bernat, linux-usb, linux-kernel, kernel-janitors
On Mon, Mar 29, 2021 at 05:43:23PM +0300, Mika Westerberg wrote:
> The nvm is a separate (physical Linux) device that gets added under this
> one. It cannot be added before AFAICT.
Hum, yes, but then it is odd that a parent is holding sysfs attributes
that refer to a child.
> The code you refer actually looks like this:
>
> static ssize_t nvm_authenticate_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t count)
> {
> ...
> if (!mutex_trylock(&rt->tb->lock)) {
> ret = restart_syscall();
> goto exit_rpm;
> }
Is that lock held during tb_retimer_nvm_add() I looked for a bit and
didn't find something. So someplace more than 4 call site above
mandatory locking is being held?
static void tb_retimer_remove(struct tb_retimer *rt)
{
dev_info(&rt->dev, "retimer disconnected\n");
tb_nvm_free(rt->nvm);
device_unregister(&rt->dev);
}
Here too?
And this is why it is all trylock because it deadlocks with unregister
otherwise?
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add()
2021-03-29 14:54 ` Jason Gunthorpe
@ 2021-03-29 15:06 ` Mika Westerberg
0 siblings, 0 replies; 7+ messages in thread
From: Mika Westerberg @ 2021-03-29 15:06 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Dan Carpenter, Andreas Noever, Kranthi Kuntala, Michael Jamet,
Yehezkel Bernat, linux-usb, linux-kernel, kernel-janitors
On Mon, Mar 29, 2021 at 11:54:05AM -0300, Jason Gunthorpe wrote:
> On Mon, Mar 29, 2021 at 05:43:23PM +0300, Mika Westerberg wrote:
>
> > The nvm is a separate (physical Linux) device that gets added under this
> > one. It cannot be added before AFAICT.
>
> Hum, yes, but then it is odd that a parent is holding sysfs attributes
> that refer to a child.
Well the child (NVMem) comes from completely different subsystem that
does not have a concept of "authentication" or anythin similar. This is
what we add on top. We actually exposer two NVMem devices under each
retimer: one that is the current active one, and then the one that is
used to write the new firmware image.
> > The code you refer actually looks like this:
> >
> > static ssize_t nvm_authenticate_store(struct device *dev,
> > struct device_attribute *attr, const char *buf, size_t count)
> > {
> > ...
> > if (!mutex_trylock(&rt->tb->lock)) {
> > ret = restart_syscall();
> > goto exit_rpm;
> > }
>
> Is that lock held during tb_retimer_nvm_add() I looked for a bit and
> didn't find something. So someplace more than 4 call site above
> mandatory locking is being held?
Yes it is. It is called from tb_scan_port() where that lock is held.
> static void tb_retimer_remove(struct tb_retimer *rt)
> {
> dev_info(&rt->dev, "retimer disconnected\n");
> tb_nvm_free(rt->nvm);
> device_unregister(&rt->dev);
> }
>
> Here too?
Yes.
> And this is why it is all trylock because it deadlocks with unregister
> otherwise?
I tried to explain it in 09f11b6c99fe ("thunderbolt: Take domain lock in
switch sysfs attribute callbacks"), except that at that time we did not
have retimers exposed but the same applies here too.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add()
2021-03-29 6:07 [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add() Dan Carpenter
2021-03-29 6:08 ` [PATCH 2/2] thunderbolt: Fix off by one in tb_port_find_retimer() Dan Carpenter
2021-03-29 13:02 ` [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add() Jason Gunthorpe
@ 2021-03-30 10:41 ` Mika Westerberg
2 siblings, 0 replies; 7+ messages in thread
From: Mika Westerberg @ 2021-03-30 10:41 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andreas Noever, Kranthi Kuntala, Michael Jamet, Yehezkel Bernat,
linux-usb, linux-kernel, kernel-janitors, Jason Gunthorpe
On Mon, Mar 29, 2021 at 09:07:18AM +0300, Dan Carpenter wrote:
> After the device_register() succeeds, then the correct way to clean up
> is to call device_unregister(). The unregister calls both device_del()
> and device_put(). Since this code was only device_del() it results in
> a memory leak.
>
> Fixes: dacb12877d92 ("thunderbolt: Add support for on-board retimers")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This is from a new static checker warning. Not tested. With new
> warnings it's also possible that I have misunderstood something
> fundamental so review carefully etc.
>
> drivers/thunderbolt/retimer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Both patches applied to thunderbolt.git/fixes. Thanks Dan!
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-30 10:42 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-29 6:07 [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add() Dan Carpenter
2021-03-29 6:08 ` [PATCH 2/2] thunderbolt: Fix off by one in tb_port_find_retimer() Dan Carpenter
2021-03-29 13:02 ` [PATCH 1/2] thunderbolt: Fix a leak in tb_retimer_add() Jason Gunthorpe
2021-03-29 14:43 ` Mika Westerberg
2021-03-29 14:54 ` Jason Gunthorpe
2021-03-29 15:06 ` Mika Westerberg
2021-03-30 10:41 ` Mika Westerberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).