[PATCH] nvmet: fix null-pointer when removing a referral

[PATCH] nvmet: fix null-pointer when removing a referral

[Talker Alex]

nvmet_referral_release() was called after item->ci_parent
and item->ci_group were set to NULL by configfs internals.
This caused oops on older kernels and possibly on the mainline too.

Tested on CentOS 7.7 (kernel 3.10) and Ubuntu 18.04.3 (kernel 4.15)
by means of MLNX OFED sources.

This patch is mainly wanted in Mellanox OFED as in-tree nvmet.ko for
mentioned kernel behaves proper as the bug was introduced about
a year ago.

I haven't found information about bug-reporting into the Mellanox OFED
but after taking a look in the mainline I thought that it might need
this patch too.

I have never before sent a kernel patch so
feel free to tell me if I did something improper.

Signed-off-by: Aleksandr Diadiushkin <alextalker@ya.ru>
---
 drivers/nvme/target/configfs.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 98613a45bd3b..00f30ab40e69 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -970,17 +970,19 @@ static struct configfs_attribute *nvmet_referral_attrs[] = {
 	NULL,
 };
 
-static void nvmet_referral_release(struct config_item *item)
+static void nvmet_referral_release(struct config_group *group,
+		struct config_item *item)
 {
-	struct nvmet_port *parent = to_nvmet_port(item->ci_parent->ci_parent);
+	struct nvmet_port *parent = to_nvmet_port(group->cg_item.ci_parent);
 	struct nvmet_port *port = to_nvmet_port(item);
 
 	nvmet_referral_disable(parent, port);
 	kfree(port);
+
+	config_item_put(item);
 }
 
 static struct configfs_item_operations nvmet_referral_item_ops = {
-	.release	= nvmet_referral_release,
 };
 
 static const struct config_item_type nvmet_referral_type = {
@@ -1006,6 +1008,7 @@ static struct config_group *nvmet_referral_make(
 
 static struct configfs_group_operations nvmet_referral_group_ops = {
 	.make_group		= nvmet_referral_make,
+	.drop_item		= nvmet_referral_release,
 };
 
 static const struct config_item_type nvmet_referrals_type = {
-- 
2.17.1



_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: fix null-pointer when removing a referral

[Sagi Grimberg]

> nvmet_referral_release() was called after item->ci_parent
> and item->ci_group were set to NULL by configfs internals.
> This caused oops on older kernels and possibly on the mainline too.
> 
> Tested on CentOS 7.7 (kernel 3.10) and Ubuntu 18.04.3 (kernel 4.15)
> by means of MLNX OFED sources.

No need to mention irrelevant distro you tested.

The below section should not exist in the change-log, so you can place
it under the "---" separator.

> 
> This patch is mainly wanted in Mellanox OFED as in-tree nvmet.ko for
> mentioned kernel behaves proper as the bug was introduced about
> a year ago.
> 
> I haven't found information about bug-reporting into the Mellanox OFED
> but after taking a look in the mainline I thought that it might need
> this patch too.
> 
> I have never before sent a kernel patch so
> feel free to tell me if I did something improper.
> 
> Signed-off-by: Aleksandr Diadiushkin <alextalker@ya.ru>
> ---
>   drivers/nvme/target/configfs.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 98613a45bd3b..00f30ab40e69 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -970,17 +970,19 @@ static struct configfs_attribute *nvmet_referral_attrs[] = {
>   	NULL,
>   };
>   
> -static void nvmet_referral_release(struct config_item *item)
> +static void nvmet_referral_release(struct config_group *group,
> +		struct config_item *item)
>   {
> -	struct nvmet_port *parent = to_nvmet_port(item->ci_parent->ci_parent);
> +	struct nvmet_port *parent = to_nvmet_port(group->cg_item.ci_parent);
>   	struct nvmet_port *port = to_nvmet_port(item);
>   
>   	nvmet_referral_disable(parent, port);
>   	kfree(port);
> +
> +	config_item_put(item);
>   }
>   
>   static struct configfs_item_operations nvmet_referral_item_ops = {
> -	.release	= nvmet_referral_release,
>   };
>   
>   static const struct config_item_type nvmet_referral_type = {
> @@ -1006,6 +1008,7 @@ static struct config_group *nvmet_referral_make(
>   
>   static struct configfs_group_operations nvmet_referral_group_ops = {
>   	.make_group		= nvmet_referral_make,
> +	.drop_item		= nvmet_referral_release,
>   };
>   
>   static const struct config_item_type nvmet_referrals_type = {
> 

Looks good,

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: fix null-pointer when removing a referral

[Talker Alex]

>
> No need to mention irrelevant distro you tested.
>

I just wanted to make a point that older software(native distro kernel)
has no such problem but the MLNX OFED back-port does.
Also, mail list rules as I remember said to test patches before sending,
so I mentioned where precisely I tested it.

> The below section should not exist in the change-log, so you can place
> it under the "---" separator.

So, the correct version of message body would look like that?

>nvmet_referral_release() was called after item->ci_parent
>and item->ci_group were set to NULL by configfs internals.
>This caused oops on older kernels and possibly on the mainline too.
>
>Signed-off-by: Aleksandr Diadiushkin <alextalker@ya.ru>
>---
>
>Tested on CentOS 7.7 (kernel 3.10) and Ubuntu 18.04.3 (kernel 4.15)
>by means of MLNX OFED sources.
>
>This patch is mainly wanted in Mellanox OFED as in-tree nvmet.ko for
>mentioned kernel behaves proper as the bug was introduced about
>a year ago.
>
>I haven't found information about bug-reporting into the Mellanox OFED
>but after taking a look in the mainline I thought that it might need
>this patch too.
>
> drivers/nvme/target/configfs.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
>index 98613a45bd3b..00f30ab40e69 100644
>--- a/drivers/nvme/target/configfs.c
>+++ b/drivers/nvme/target/configfs.c
>@@ -970,17 +970,19 @@ static struct configfs_attribute *nvmet_referral_attrs[] = {
> 	NULL,
> };
> 
>-static void nvmet_referral_release(struct config_item *item)
>+static void nvmet_referral_release(struct config_group *group,
>+		struct config_item *item)
> {
>-	struct nvmet_port *parent = to_nvmet_port(item->ci_parent->ci_parent);
>+	struct nvmet_port *parent = to_nvmet_port(group->cg_item.ci_parent);
> 	struct nvmet_port *port = to_nvmet_port(item);
> 
> 	nvmet_referral_disable(parent, port);
> 	kfree(port);
>+
>+	config_item_put(item);
> }
> 
> static struct configfs_item_operations nvmet_referral_item_ops = {
>-	.release	= nvmet_referral_release,
> };
> 
> static const struct config_item_type nvmet_referral_type = {
>@@ -1006,6 +1008,7 @@ static struct config_group *nvmet_referral_make(
> 
> static struct configfs_group_operations nvmet_referral_group_ops = {
> 	.make_group		= nvmet_referral_make,
>+	.drop_item		= nvmet_referral_release,
> };
> 
> static const struct config_item_type nvmet_referrals_type = {
>-- 
>2.17.1

Also, am I correctly understood that now I just need to wait for this patch to be accepted?
Or there required some activity on my side to make it happen?
My main goal is to get nvmet module provided by Mellanox OFED fixed
but I'm do know nothing about its development process. I saw some people from
the company mentioned here, so this might be it.


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: fix null-pointer when removing a referral

[Christoph Hellwig]

On Fri, Dec 06, 2019 at 11:39:53PM +0300, Talker Alex wrote:
> nvmet_referral_release() was called after item->ci_parent
> and item->ci_group were set to NULL by configfs internals.
> This caused oops on older kernels and possibly on the mainline too.
> 
> Tested on CentOS 7.7 (kernel 3.10) and Ubuntu 18.04.3 (kernel 4.15)
> by means of MLNX OFED sources.
> 
> This patch is mainly wanted in Mellanox OFED as in-tree nvmet.ko for
> mentioned kernel behaves proper as the bug was introduced about
> a year ago.
> 
> I haven't found information about bug-reporting into the Mellanox OFED
> but after taking a look in the mainline I thought that it might need
> this patch too.
> 
> I have never before sent a kernel patch so
> feel free to tell me if I did something improper.

As Sagi said the commit description can be shorted, basically just
the first paragraph is needed.

>  static struct configfs_item_operations nvmet_referral_item_ops = {
> -	.release	= nvmet_referral_release,
>  };

nvmet_referral_item_ops can be entirely removed now.

Otherwise the patch looks good, thanks a lot!

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: fix null-pointer when removing a referral

[Talker Alex]

nvmet_referral_release() was called when item->ci_parent
or item->ci_group were already set to NULL by configfs internals.

Signed-off-by: Aleksandr Diadiushkin <alextalker@ya.ru>
---
 drivers/nvme/target/configfs.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 98613a45bd3b..20e4a660684f 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -970,23 +970,21 @@ static struct configfs_attribute *nvmet_referral_attrs[] = {
 	NULL,
 };
 
-static void nvmet_referral_release(struct config_item *item)
+static void nvmet_referral_release(struct config_group *group,
+		struct config_item *item)
 {
-	struct nvmet_port *parent = to_nvmet_port(item->ci_parent->ci_parent);
+	struct nvmet_port *parent = to_nvmet_port(group->cg_item.ci_parent);
 	struct nvmet_port *port = to_nvmet_port(item);
 
 	nvmet_referral_disable(parent, port);
 	kfree(port);
-}
 
-static struct configfs_item_operations nvmet_referral_item_ops = {
-	.release	= nvmet_referral_release,
-};
+	config_item_put(item);
+}
 
 static const struct config_item_type nvmet_referral_type = {
 	.ct_owner	= THIS_MODULE,
 	.ct_attrs	= nvmet_referral_attrs,
-	.ct_item_ops	= &nvmet_referral_item_ops,
 };
 
 static struct config_group *nvmet_referral_make(
@@ -1006,6 +1004,7 @@ static struct config_group *nvmet_referral_make(
 
 static struct configfs_group_operations nvmet_referral_group_ops = {
 	.make_group		= nvmet_referral_make,
+	.drop_item		= nvmet_referral_release,
 };
 
 static const struct config_item_type nvmet_referrals_type = {
-- 
2.17.1

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: fix null-pointer when removing a referral

[Talker Alex]

> nvmet_referral_item_ops can be entirely removed now.

I was wondering if removing the nvmet_referral_item_ops can cause NULL reference again
but things went ain't so bad.

> Otherwise the patch looks good, thanks a lot!

Thanks a lot for responding on this. This patch really matters to me,
as everything else in configuration seems to be working out quite fine.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: fix null-pointer when removing a referral

[Christoph Hellwig]

On Thu, Dec 12, 2019 at 01:36:28PM +0300, Talker Alex wrote:
> nvmet_referral_release() was called when item->ci_parent
> or item->ci_group were already set to NULL by configfs internals.
> 
> Signed-off-by: Aleksandr Diadiushkin <alextalker@ya.ru>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: fix null-pointer when removing a referral

[Talker Alex]

Ping.

Would anyone kindly finally merge this fix?

I'd really appreciate it.

12.12.2019, 13:43, "Christoph Hellwig" <hch@infradead.org>:
> On Thu, Dec 12, 2019 at 01:36:28PM +0300, Talker Alex wrote:
>>  nvmet_referral_release() was called when item->ci_parent
>>  or item->ci_group were already set to NULL by configfs internals.
>>
>>  Signed-off-by: Aleksandr Diadiushkin <alextalker@ya.ru>
>
> Looks good,
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: fix null-pointer when removing a referral

[Keith Busch]

On Thu, Jan 16, 2020 at 05:15:40PM +0300, Talker Alex wrote:
> Ping.
> 
> Would anyone kindly finally merge this fix?

Patch applied for-5.6, thank you for the ping. 
 
> 12.12.2019, 13:43, "Christoph Hellwig" <hch@infradead.org>:
> > On Thu, Dec 12, 2019 at 01:36:28PM +0300, Talker Alex wrote:
> >>  nvmet_referral_release() was called when item->ci_parent
> >>  or item->ci_group were already set to NULL by configfs internals.
> >>
> >>  Signed-off-by: Aleksandr Diadiushkin <alextalker@ya.ru>
> >
> > Looks good,
> >
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> _______________________________________________
> linux-nvme mailing list
> linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme