[PATCH 0/4] RDMA/srp: Handle dev_set_name() failure

[PATCH 0/4] RDMA/srp: Handle dev_set_name() failure

[Bart Van Assche]

Hi Jason,

This patch series includes one patch that handles dev_set_name() failure and
three refactoring patches. Please consider these patches for the next merge
window.

Thanks,

Bart.

Bart Van Assche (4):
  RDMA/srp: Rework the srp_add_port() error path
  RDMA/srp: Remove the srp_host.released completion
  RDMA/srp: Handle dev_set_name() failure
  RDMA/srp: Use the attribute group mechanism for sysfs attributes

 drivers/infiniband/ulp/srp/ib_srp.c | 50 +++++++++++++++--------------
 drivers/infiniband/ulp/srp/ib_srp.h |  1 -
 2 files changed, 26 insertions(+), 25 deletions(-)

[PATCH 1/4] RDMA/srp: Rework the srp_add_port() error path

[Bart Van Assche]

device_register() always calls device_initialize() so calling device_del()
is safe even if device_register() fails. Implement the following advice
from the comment block above device_register(): "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."
Keep the kfree() call in the error path since srp_release_dev() does not
free the host.

Cc: stable@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 7720ea270ed8..8fd6a88f7a9c 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3909,20 +3909,19 @@ static struct srp_host *srp_add_port(struct srp_device *device, u8 port)
 		     port);
 
 	if (device_register(&host->dev))
-		goto free_host;
+		goto put_host;
 	if (device_create_file(&host->dev, &dev_attr_add_target))
-		goto err_class;
+		goto put_host;
 	if (device_create_file(&host->dev, &dev_attr_ibdev))
-		goto err_class;
+		goto put_host;
 	if (device_create_file(&host->dev, &dev_attr_port))
-		goto err_class;
+		goto put_host;
 
 	return host;
 
-err_class:
-	device_unregister(&host->dev);
-
-free_host:
+put_host:
+	device_del(&host->dev);
+	put_device(&host->dev);
 	kfree(host);
 
 	return NULL;

[PATCH 2/4] RDMA/srp: Remove the srp_host.released completion

[Bart Van Assche]

Move the kfree(host) calls into srp_release_dev(). Convert a
device_unregister() call into a device_del() and a device_put() call.
Remove the host->released completion object. This patch prepares for
handling dev_set_name() failure in srp_add_port().

Cc: stable@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 14 +++++---------
 drivers/infiniband/ulp/srp/ib_srp.h |  1 -
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 8fd6a88f7a9c..1d3a15e63732 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3178,7 +3178,7 @@ static void srp_release_dev(struct device *dev)
 	struct srp_host *host =
 		container_of(dev, struct srp_host, dev);
 
-	complete(&host->released);
+	kfree(host);
 }
 
 static struct class srp_class = {
@@ -3898,7 +3898,6 @@ static struct srp_host *srp_add_port(struct srp_device *device, u8 port)
 
 	INIT_LIST_HEAD(&host->target_list);
 	spin_lock_init(&host->target_lock);
-	init_completion(&host->released);
 	mutex_init(&host->add_target_mutex);
 	host->srp_dev = device;
 	host->port = port;
@@ -3922,8 +3921,6 @@ static struct srp_host *srp_add_port(struct srp_device *device, u8 port)
 put_host:
 	device_del(&host->dev);
 	put_device(&host->dev);
-	kfree(host);
-
 	return NULL;
 }
 
@@ -4029,12 +4026,11 @@ static void srp_remove_one(struct ib_device *device, void *client_data)
 	srp_dev = client_data;
 
 	list_for_each_entry_safe(host, tmp_host, &srp_dev->dev_list, list) {
-		device_unregister(&host->dev);
 		/*
-		 * Wait for the sysfs entry to go away, so that no new
-		 * target ports can be created.
+		 * Remove the add_target sysfs entry so that no new target ports
+		 * can be created.
 		 */
-		wait_for_completion(&host->released);
+		device_del(&host->dev);
 
 		/*
 		 * Remove all target ports.
@@ -4052,7 +4048,7 @@ static void srp_remove_one(struct ib_device *device, void *client_data)
 		 */
 		flush_workqueue(srp_remove_wq);
 
-		kfree(host);
+		put_device(&host->dev);
 	}
 
 	ib_dealloc_pd(srp_dev->pd);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 55a575e2cace..493e7fd1913e 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -124,7 +124,6 @@ struct srp_host {
 	struct device		dev;
 	struct list_head	target_list;
 	spinlock_t		target_lock;
-	struct completion	released;
 	struct list_head	list;
 	struct mutex		add_target_mutex;
 };

[PATCH 3/4] RDMA/srp: Handle dev_set_name() failure

[Bart Van Assche]

Instead of ignoring dev_set_name() failure, handle dev_set_name()
failure. Convert a device_register() call into device_initialize() and
device_add() calls.

Reported-by: Bo Liu <liubo03@inspur.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 1d3a15e63732..3f31a0eef1ef 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3902,12 +3902,13 @@ static struct srp_host *srp_add_port(struct srp_device *device, u8 port)
 	host->srp_dev = device;
 	host->port = port;
 
+	device_initialize(&host->dev);
 	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 (device_register(&host->dev))
+	if (dev_set_name(&host->dev, "srp-%s-%d", dev_name(&device->dev->dev),
+			 port))
+		goto put_host;
+	if (device_add(&host->dev))
 		goto put_host;
 	if (device_create_file(&host->dev, &dev_attr_add_target))
 		goto put_host;

[PATCH 4/4] RDMA/srp: Use the attribute group mechanism for sysfs attributes

[Bart Van Assche]

Simplify the SRP driver by using the attribute group mechanism instead
of calling device_create_file() explicitly.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 3f31a0eef1ef..1e777b2043d6 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3181,8 +3181,13 @@ static void srp_release_dev(struct device *dev)
 	kfree(host);
 }
 
+static struct attribute *srp_class_attrs[];
+
+ATTRIBUTE_GROUPS(srp_class);
+
 static struct class srp_class = {
 	.name    = "infiniband_srp",
+	.dev_groups = srp_class_groups,
 	.dev_release = srp_release_dev
 };
 
@@ -3888,6 +3893,13 @@ static ssize_t port_show(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR_RO(port);
 
+static struct attribute *srp_class_attrs[] = {
+	&dev_attr_add_target.attr,
+	&dev_attr_ibdev.attr,
+	&dev_attr_port.attr,
+	NULL
+};
+
 static struct srp_host *srp_add_port(struct srp_device *device, u8 port)
 {
 	struct srp_host *host;
@@ -3910,12 +3922,6 @@ static struct srp_host *srp_add_port(struct srp_device *device, u8 port)
 		goto put_host;
 	if (device_add(&host->dev))
 		goto put_host;
-	if (device_create_file(&host->dev, &dev_attr_add_target))
-		goto put_host;
-	if (device_create_file(&host->dev, &dev_attr_ibdev))
-		goto put_host;
-	if (device_create_file(&host->dev, &dev_attr_port))
-		goto put_host;
 
 	return host;
 

Re: [PATCH 0/4] RDMA/srp: Handle dev_set_name() failure

[Leon Romanovsky]

On Thu, Aug 25, 2022 at 02:38:56PM -0700, Bart Van Assche wrote:
> Hi Jason,
> 
> This patch series includes one patch that handles dev_set_name() failure and
> three refactoring patches. Please consider these patches for the next merge
> window.

You confuse me. "next merge window" means that patches are targeted to
-next, but you added stable@... tag and didn't add any Fixes lines.

I applied everything to rdma-next and removed stable@ tag.

Thanks

> 
> Thanks,
> 
> Bart.
> 
> Bart Van Assche (4):
>   RDMA/srp: Rework the srp_add_port() error path
>   RDMA/srp: Remove the srp_host.released completion
>   RDMA/srp: Handle dev_set_name() failure
>   RDMA/srp: Use the attribute group mechanism for sysfs attributes
> 
>  drivers/infiniband/ulp/srp/ib_srp.c | 50 +++++++++++++++--------------
>  drivers/infiniband/ulp/srp/ib_srp.h |  1 -
>  2 files changed, 26 insertions(+), 25 deletions(-)
> 

Re: [PATCH 0/4] RDMA/srp: Handle dev_set_name() failure

[Bart Van Assche]

On 8/28/22 03:04, Leon Romanovsky wrote:
> On Thu, Aug 25, 2022 at 02:38:56PM -0700, Bart Van Assche wrote:
>> This patch series includes one patch that handles dev_set_name() failure and
>> three refactoring patches. Please consider these patches for the next merge
>> window.
> 
> You confuse me. "next merge window" means that patches are targeted to
> -next, but you added stable@... tag and didn't add any Fixes lines.
> 
> I applied everything to rdma-next and removed stable@ tag.

Hi Leon,

Although it's not a big deal for this patch series, please do not modify patches
without agreement from the patch author.

As far as I know adding a Fixes: tag if a Cc: stable tag is present is not required
by any document in the Documentation/ directory?

I had not added a Fixes: tag because the issue fixed by patch 3/3 was introduced
by the commit that added the ib_srp driver to the kernel tree. So it would be fine
to backport the first three patches of this series to all older kernel versions to
which the patches can be backported.

Thanks,

Bart.

Re: [PATCH 0/4] RDMA/srp: Handle dev_set_name() failure

[Leon Romanovsky]

On Sun, Aug 28, 2022 at 12:50:28PM -0700, Bart Van Assche wrote:
> On 8/28/22 03:04, Leon Romanovsky wrote:
> > On Thu, Aug 25, 2022 at 02:38:56PM -0700, Bart Van Assche wrote:
> > > This patch series includes one patch that handles dev_set_name() failure and
> > > three refactoring patches. Please consider these patches for the next merge
> > > window.
> > 
> > You confuse me. "next merge window" means that patches are targeted to
> > -next, but you added stable@... tag and didn't add any Fixes lines.
> > 
> > I applied everything to rdma-next and removed stable@ tag.
> 
> Hi Leon,
> 
> Although it's not a big deal for this patch series, please do not modify patches
> without agreement from the patch author.

I didn't promote the series from my WIP branch to for-next yet and can drop
them, if you want.

> 
> As far as I know adding a Fixes: tag if a Cc: stable tag is present is not required
> by any document in the Documentation/ directory?
> 
> I had not added a Fixes: tag because the issue fixed by patch 3/3 was introduced
> by the commit that added the ib_srp driver to the kernel tree. So it would be fine
> to backport the first three patches of this series to all older kernel versions to
> which the patches can be backported.

You wanted third patch in stable@, but didn't add tag to it or any
indication that it must be there. Instead of it, you added stable@
to some cleanup that would be backported anyway if third patch would
be stable material.

Let's me cite Documentation/process/stable-kernel-rules.rst with items
that make this series is not suitable for stable:
...
   12  - It must fix a real bug that bothers people (not a, "This could be a
   13    problem..." type thing).
   14  - It must fix a problem that causes a build error (but not for things
   15    marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
   16    security issue, or some "oh, that's not good" issue.  In short, something
   17    critical.
   18  - Serious issues as reported by a user of a distribution kernel may also
   19    be considered if they fix a notable performance or interactivity issue.
   20    As these fixes are not as obvious and have a higher risk of a subtle
   21    regression they should only be submitted by a distribution kernel
   22    maintainer and include an addendum linking to a bugzilla entry if it
   23    exists and additional information on the user-visible impact.
   24  - New device IDs and quirks are also accepted.
...
   25  - No "theoretical race condition" issues, unless an explanation of how the
   26    race can be exploited is also provided.
   29  - It must follow the 
   30    :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
   31    rules.

Documentation/process/submitting-patches.rst:
...
  137 If your patch fixes a bug in a specific commit, e.g. you found an issue using
  138 ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
  139 the SHA-1 ID, and the one line summary. 
...

Also I hope that you looked when dev_set_name() can fail. Hint, when it
failed to allocate enough room for short string "srp-%s-%d". If it is
happened, you have much more serious problems than not-checked
dev_set_name().

Why is it so urgent to be part of stable? Can you present me the case
where user had OOM during dev_set_name at the beginning of srp initialization
routine and passed device_register() later?

Thanks

> 
> Thanks,
> 
> Bart.

Re: [PATCH 0/4] RDMA/srp: Handle dev_set_name() failure

[Jason Gunthorpe]

On Sun, Aug 28, 2022 at 12:50:28PM -0700, Bart Van Assche wrote:
> On 8/28/22 03:04, Leon Romanovsky wrote:
> > On Thu, Aug 25, 2022 at 02:38:56PM -0700, Bart Van Assche wrote:
> > > This patch series includes one patch that handles dev_set_name() failure and
> > > three refactoring patches. Please consider these patches for the next merge
> > > window.
> > 
> > You confuse me. "next merge window" means that patches are targeted to
> > -next, but you added stable@... tag and didn't add any Fixes lines.
> > 
> > I applied everything to rdma-next and removed stable@ tag.
> 
> Hi Leon,
> 
> Although it's not a big deal for this patch series, please do not modify patches
> without agreement from the patch author.
> 
> As far as I know adding a Fixes: tag if a Cc: stable tag is present is not required
> by any document in the Documentation/ directory?
> 
> I had not added a Fixes: tag because the issue fixed by patch 3/3 was introduced
> by the commit that added the ib_srp driver to the kernel tree. So it would be fine
> to backport the first three patches of this series to all older kernel versions to
> which the patches can be backported.

Generally I always want to see the Fixes tag because it aides everyone
in the ecosystem to know when they should consider the patch for
backporting with a simple uniform algorithm.

So marking the first commit as the thing being fixed is expected.

Jason