| Message ID | 20220805053434.3944-1-liubo03@inspur.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RDMA/srp: Check dev_set_name() return value | expand |
On 8/4/22 22:34, Bo Liu wrote: > It's possible that dev_set_name() returns -ENOMEM, catch and handle this. > > Signed-off-by: Bo Liu <liubo03@inspur.com> > --- > drivers/infiniband/ulp/srp/ib_srp.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c > index 7720ea270ed8..a6f788e3b84b 100644 > --- a/drivers/infiniband/ulp/srp/ib_srp.c > +++ b/drivers/infiniband/ulp/srp/ib_srp.c > @@ -3905,8 +3905,9 @@ static struct srp_host *srp_add_port(struct srp_device *device, u8 port) > > host->dev.class = &srp_class; > host->dev.parent = device->dev->dev.parent; > - dev_set_name(&host->dev, "srp-%s-%d", dev_name(&device->dev->dev), > - port); > + if (dev_set_name(&host->dev, "srp-%s-%d", dev_name(&device->dev->dev), > + port)) > + goto free_host; > > if (device_register(&host->dev)) > goto free_host; Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Fri, Aug 05, 2022 at 01:34:34AM -0400, Bo Liu wrote: > It's possible that dev_set_name() returns -ENOMEM, catch and handle this. > > Signed-off-by: Bo Liu <liubo03@inspur.com> > --- > drivers/infiniband/ulp/srp/ib_srp.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Ah, while you are here can you fix this: > if (device_register(&host->dev)) > goto free_host; [..] > free_host: > kfree(host); That isn't allowed, you have to do put_device(): /** * device_register - register a device with the system. * @dev: pointer to the device structure * * This happens in two clean steps - initialize the device * and add it to the system. The two steps can be called * separately, but this is the easiest and most common. * I.e. you should only call the two helpers separately if * have a clearly defined need to use and refcount the device * before it is added to the hierarchy. * * For more information, see the kerneldoc for device_initialize() * and device_add(). * * NOTE: _Never_ directly free @dev after calling this function, even * if it returned an error! Always use put_device() to give up the * reference initialized in this function instead. */ int device_register(struct device *dev) Everyone get this wrong, it is why I think device_register is a terrible interface. Write it as device_initialize()/device_add() as seperate steps and never call kfree(). Jason
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 7720ea270ed8..a6f788e3b84b 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -3905,8 +3905,9 @@ static struct srp_host *srp_add_port(struct srp_device *device, u8 port) host->dev.class = &srp_class; host->dev.parent = device->dev->dev.parent; - dev_set_name(&host->dev, "srp-%s-%d", dev_name(&device->dev->dev), - port); + if (dev_set_name(&host->dev, "srp-%s-%d", dev_name(&device->dev->dev), + port)) + goto free_host; if (device_register(&host->dev)) goto free_host;
It's possible that dev_set_name() returns -ENOMEM, catch and handle this. Signed-off-by: Bo Liu <liubo03@inspur.com> --- drivers/infiniband/ulp/srp/ib_srp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)