thunderbolt: Fix a leak in tb_retimer_add()
Subsystems (3)
PATCH 1/2 thunderbolt: Fix a leak in tb_retimer_add()
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.
---
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(-)
drivers/thunderbolt/retimer.c +1 -1
@@ -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; }
Signature · 1 line
2.30.2
PATCH 2/2 thunderbolt: Fix off by one in tb_port_find_retimer()
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.
--- drivers/thunderbolt/retimer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/thunderbolt/retimer.c +1 -1
@@ -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)
Signature · 1 line
2.30.2
Quoted 11 lines · Dan Carpenter
After the
device_register()succeeds, then the correct way to clean up is to calldevice_unregister(). The unregister calls bothdevice_del()anddevice_put(). Since this code was onlydevice_del()it results in a memory leak.Fixes dacb12877d92 ("thunderbolt: Add support for on-board retimers")Signed-off-by Dan Carpenter <[email protected]>---
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
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
Hi,
Quoted 6 lines · Jason Gunthorpe
Quoted 5 lines · Dan Carpenter
After the
device_register()succeeds, then the correct way to clean up is to calldevice_unregister(). The unregister calls bothdevice_del()anddevice_put(). Since this code was onlydevice_del()it results in a memory leak.
Quoted 8 lines
Quoted 6 lines
Fixes dacb12877d92 ("thunderbolt: Add support for on-board retimers")Signed-off-by Dan Carpenter <[email protected]>---
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.
Quoted 1 line
Reviewed-by Jason Gunthorpe <[email protected]>
Thanks for the review!
Quoted 10 lines
This also highlights the code has an ordering issue too, it calls
device_register()then goes to dotb_retimer_nvm_add()howeverdevice_register()makes sysfs attributes visible before thert->nvmis initialized and this:static
ssize_tnvm_authenticate_store(struct device *dev,
structdevice_attribute*attr, const char *buf,size_tcount){ if (!rt->nvm) {
Quoted 7 lines
Isn't strong enough to close the potential racing. The nvm should be setup before
device_registerand all the above tests in the sysfs deleted so we can rely on the CPU barriers built intodevice_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.
Quoted 2 lines · Mika Westerberg
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.
Quoted 10 lines
The code you refer actually looks like this:
static
ssize_tnvm_authenticate_store(struct device *dev,
structdevice_attribute*attr, const char *buf,size_tcount){ ... 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
Quoted 7 lines · Jason Gunthorpe
Quoted 2 lines · Mika Westerberg
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.
Quoted 11 lines
Quoted 10 lines
The code you refer actually looks like this:
static
ssize_tnvm_authenticate_store(struct device *dev,
structdevice_attribute*attr, const char *buf,size_tcount){ ... if (!mutex_trylock(&rt->tb->lock)) { ret = restart_syscall(); goto exit_rpm; }
Quoted 3 lines
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.
Quoted 8 lines
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.
Quoted 2 lines
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.
Quoted 7 lines · Dan Carpenter
After the
device_register()succeeds, then the correct way to clean up is to calldevice_unregister(). The unregister calls bothdevice_del()anddevice_put(). Since this code was onlydevice_del()it results in a memory leak.Fixes dacb12877d92 ("thunderbolt: Add support for on-board retimers")Signed-off-by Dan Carpenter <[email protected]>
Quoted 7 lines
---
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!