| Message ID | 20191121213628.21244-1-rrichter@marvell.com |
|---|---|
| State | Accepted |
| Commit | 16214bd9e43a31683a7073664b000029bba00354 |
| Headers | show |
| Series | EDAC/ghes: Do not warn when incrementing refcount on 0 | expand |
On Thu, Nov 21, 2019 at 09:36:57PM +0000, Robert Richter wrote: > Following warning from the refcount framework is seen during ghes > initialization: > > EDAC MC0: Giving out device to module ghes_edac.c controller ghes_edac: DEV ghes (INTERRUPT) > ------------[ cut here ]------------ > refcount_t: increment on 0; use-after-free. > WARNING: CPU: 36 PID: 1 at lib/refcount.c:156 refcount_inc_checked+0x44/0x50 > [...] > Call trace: > refcount_inc_checked+0x44/0x50 > ghes_edac_register+0x258/0x388 > ghes_probe+0x28c/0x5f0 > > It warns if the refcount is incremented from zero. This warning is > reasonable as a kernel object is typically created with a refcount of > one and freed once the refcount is zero. Afterwards the object would > be "used-after-free". > > For ghes the refcount is initialized with zero, and that is why this > message is seen when initializing the first instance. However, > whenever the refcount is zero, the device will be allocated and > registered. Since the ghes_reg_mutex protects the refcount and > serializes allocation and freeing of ghes devices, a use-after-free > cannot happen here. > > Instead of using refcount_inc() for the first instance, use > refcount_set(). This can be used here because the refcount is zero at > this point and can not change due to its protection by the mutex. > > Reported-by: John Garry <john.garry@huawei.com> > Tested-by: John Garry <john.garry@huawei.com> > Signed-off-by: Robert Richter <rrichter@marvell.com> > --- > drivers/edac/ghes_edac.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Queued, thanks. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
On 22/11/2019 09:01, Borislav Petkov wrote: > On Thu, Nov 21, 2019 at 09:36:57PM +0000, Robert Richter wrote: >> Following warning from the refcount framework is seen during ghes >> initialization: >> >> EDAC MC0: Giving out device to module ghes_edac.c controller ghes_edac: DEV ghes (INTERRUPT) >> ------------[ cut here ]------------ >> refcount_t: increment on 0; use-after-free. >> WARNING: CPU: 36 PID: 1 at lib/refcount.c:156 refcount_inc_checked+0x44/0x50 >> [...] >> Call trace: >> refcount_inc_checked+0x44/0x50 >> ghes_edac_register+0x258/0x388 >> ghes_probe+0x28c/0x5f0 >> >> It warns if the refcount is incremented from zero. This warning is >> reasonable as a kernel object is typically created with a refcount of >> one and freed once the refcount is zero. Afterwards the object would >> be "used-after-free". >> >> For ghes the refcount is initialized with zero, and that is why this >> message is seen when initializing the first instance. However, >> whenever the refcount is zero, the device will be allocated and >> registered. Since the ghes_reg_mutex protects the refcount and >> serializes allocation and freeing of ghes devices, a use-after-free >> cannot happen here. >> >> Instead of using refcount_inc() for the first instance, use >> refcount_set(). This can be used here because the refcount is zero at >> this point and can not change due to its protection by the mutex. >> >> Reported-by: John Garry <john.garry@huawei.com> >> Tested-by: John Garry <john.garry@huawei.com> According to kernel dev process Doc, this should be explicitly granted, so: Tested-by: John Garry <john.garry@huawei.com> Thanks, John >> Signed-off-by: Robert Richter <rrichter@marvell.com> >> --- >> drivers/edac/ghes_edac.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) > > Queued, thanks. >
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index 47f4e7f90ef0..b99080d8a10c 100644 --- a/drivers/edac/ghes_edac.c +++ b/drivers/edac/ghes_edac.c @@ -556,8 +556,8 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) ghes_pvt = pvt; spin_unlock_irqrestore(&ghes_lock, flags); - /* only increment on success */ - refcount_inc(&ghes_refcount); + /* only set on success */ + refcount_set(&ghes_refcount, 1); unlock: mutex_unlock(&ghes_reg_mutex);