[PATCH rdma-next 0/4] Various core fixes

[PATCH rdma-next 0/4] Various core fixes

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Batch unrelated fixes.

Mark Zhang (3):
  RDMA/restrack: Release MR restrack when delete
  RDMA/core: Make sure "ib_port" is valid when access sysfs node
  RDMA/nldev: Return "-EAGAIN" if the cm_id isn't from expected port

Or Har-Toov (1):
  RDMA/nldev: Use __nlmsg_put instead nlmsg_put

 drivers/infiniband/core/nldev.c    | 110 ++++++++++++++---------------
 drivers/infiniband/core/restrack.c |   2 -
 drivers/infiniband/core/sysfs.c    |  17 +++--
 3 files changed, 64 insertions(+), 65 deletions(-)

-- 
2.38.1

[PATCH rdma-next 1/4] RDMA/nldev: Use __nlmsg_put instead nlmsg_put

[Leon Romanovsky]

From: Or Har-Toov <ohartoov@nvidia.com>

Using nlmsg_put causes static analysis tools to many
false positives of not checking the return value of nlmsg_put.

In all uses in nldev.c, payload parameter is 0 so NULL will never
be returned. So let's use __nlmsg_put function to silence the
warnings.

Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/nldev.c | 108 +++++++++++++++-----------------
 1 file changed, 52 insertions(+), 56 deletions(-)

diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
index b92358f606d0..213be4e6fd28 100644
--- a/drivers/infiniband/core/nldev.c
+++ b/drivers/infiniband/core/nldev.c
@@ -1038,9 +1038,9 @@ static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto err;
 	}
 
-	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET),
-			0, 0);
+	nlh = __nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+			  RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET),
+			  0, 0);
 
 	err = fill_dev_info(msg, device);
 	if (err)
@@ -1122,9 +1122,9 @@ static int _nldev_get_dumpit(struct ib_device *device,
 	if (idx < start)
 		return 0;
 
-	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET),
-			0, NLM_F_MULTI);
+	nlh = __nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
+			  RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET),
+			  0, NLM_F_MULTI);
 
 	if (fill_dev_info(skb, device)) {
 		nlmsg_cancel(skb, nlh);
@@ -1182,9 +1182,9 @@ static int nldev_port_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto err;
 	}
 
-	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET),
-			0, 0);
+	nlh = __nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+			  RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_GET),
+			  0, 0);
 
 	err = fill_port_info(msg, device, port, sock_net(skb->sk));
 	if (err)
@@ -1240,11 +1240,11 @@ static int nldev_port_get_dumpit(struct sk_buff *skb,
 			continue;
 		}
 
-		nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid,
-				cb->nlh->nlmsg_seq,
-				RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
-						 RDMA_NLDEV_CMD_PORT_GET),
-				0, NLM_F_MULTI);
+		nlh = __nlmsg_put(skb, NETLINK_CB(cb->skb).portid,
+				  cb->nlh->nlmsg_seq,
+				  RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
+						   RDMA_NLDEV_CMD_PORT_GET),
+				  0, NLM_F_MULTI);
 
 		if (fill_port_info(skb, device, p, sock_net(skb->sk))) {
 			nlmsg_cancel(skb, nlh);
@@ -1285,9 +1285,9 @@ static int nldev_res_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto err;
 	}
 
-	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_RES_GET),
-			0, 0);
+	nlh = __nlmsg_put(
+		msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+		RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_RES_GET), 0, 0);
 
 	ret = fill_res_info(msg, device);
 	if (ret)
@@ -1315,9 +1315,10 @@ static int _nldev_res_get_dumpit(struct ib_device *device,
 	if (idx < start)
 		return 0;
 
-	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_RES_GET),
-			0, NLM_F_MULTI);
+	nlh = __nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
+			  RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
+					   RDMA_NLDEV_CMD_RES_GET),
+			  0, NLM_F_MULTI);
 
 	if (fill_res_info(skb, device)) {
 		nlmsg_cancel(skb, nlh);
@@ -1449,10 +1450,10 @@ static int res_get_common_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto err_get;
 	}
 
-	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
-					 RDMA_NL_GET_OP(nlh->nlmsg_type)),
-			0, 0);
+	nlh = __nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+			  RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
+					   RDMA_NL_GET_OP(nlh->nlmsg_type)),
+			  0, 0);
 
 	if (fill_nldev_handle(msg, device)) {
 		ret = -EMSGSIZE;
@@ -1528,10 +1529,10 @@ static int res_get_common_dumpit(struct sk_buff *skb,
 		}
 	}
 
-	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
-					 RDMA_NL_GET_OP(cb->nlh->nlmsg_type)),
-			0, NLM_F_MULTI);
+	nlh = __nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
+			  RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
+					   RDMA_NL_GET_OP(cb->nlh->nlmsg_type)),
+			  0, NLM_F_MULTI);
 
 	if (fill_nldev_handle(skb, device)) {
 		ret = -EMSGSIZE;
@@ -1791,10 +1792,10 @@ static int nldev_get_chardev(struct sk_buff *skb, struct nlmsghdr *nlh,
 		err = -ENOMEM;
 		goto out_put;
 	}
-	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
-					 RDMA_NLDEV_CMD_GET_CHARDEV),
-			0, 0);
+	nlh = __nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+			  RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
+					   RDMA_NLDEV_CMD_GET_CHARDEV),
+			  0, 0);
 
 	data.nl_msg = msg;
 	err = ib_get_client_nl_info(ibdev, client_name, &data);
@@ -1848,10 +1849,9 @@ static int nldev_sys_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (!msg)
 		return -ENOMEM;
 
-	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
-					 RDMA_NLDEV_CMD_SYS_GET),
-			0, 0);
+	nlh = __nlmsg_put(
+		msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+		RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_SYS_GET), 0, 0);
 
 	err = nla_put_u8(msg, RDMA_NLDEV_SYS_ATTR_NETNS_MODE,
 			 (u8)ib_devices_shared_netns);
@@ -2028,10 +2028,9 @@ static int nldev_stat_set_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 		ret = -ENOMEM;
 		goto err_put_device;
 	}
-	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
-					 RDMA_NLDEV_CMD_STAT_SET),
-			0, 0);
+	nlh = __nlmsg_put(
+		msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+		RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_STAT_SET), 0, 0);
 	if (fill_nldev_handle(msg, device) ||
 	    nla_put_u32(msg, RDMA_NLDEV_ATTR_PORT_INDEX, port)) {
 		ret = -EMSGSIZE;
@@ -2097,10 +2096,9 @@ static int nldev_stat_del_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 		ret = -ENOMEM;
 		goto err;
 	}
-	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
-					 RDMA_NLDEV_CMD_STAT_SET),
-			0, 0);
+	nlh = __nlmsg_put(
+		msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+		RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_STAT_SET), 0, 0);
 
 	cntn = nla_get_u32(tb[RDMA_NLDEV_ATTR_STAT_COUNTER_ID]);
 	qpn = nla_get_u32(tb[RDMA_NLDEV_ATTR_RES_LQPN]);
@@ -2166,10 +2164,9 @@ static int stat_get_doit_default_counter(struct sk_buff *skb,
 		goto err;
 	}
 
-	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
-					 RDMA_NLDEV_CMD_STAT_GET),
-			0, 0);
+	nlh = __nlmsg_put(
+		msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+		RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_STAT_GET), 0, 0);
 
 	if (fill_nldev_handle(msg, device) ||
 	    nla_put_u32(msg, RDMA_NLDEV_ATTR_PORT_INDEX, port)) {
@@ -2255,10 +2252,9 @@ static int stat_get_doit_qp(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto err;
 	}
 
-	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
-			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
-					 RDMA_NLDEV_CMD_STAT_GET),
-			0, 0);
+	nlh = __nlmsg_put(
+		msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+		RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_STAT_GET), 0, 0);
 
 	ret = rdma_counter_get_mode(device, port, &mode, &mask);
 	if (ret)
@@ -2385,10 +2381,10 @@ static int nldev_stat_get_counter_status_doit(struct sk_buff *skb,
 		goto err;
 	}
 
-	nlh = nlmsg_put(
-		msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
-		RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_STAT_GET_STATUS),
-		0, 0);
+	nlh = __nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+			  RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
+					   RDMA_NLDEV_CMD_STAT_GET_STATUS),
+			  0, 0);
 
 	ret = -EMSGSIZE;
 	if (fill_nldev_handle(msg, device) ||
-- 
2.38.1

Re: [PATCH rdma-next 1/4] RDMA/nldev: Use __nlmsg_put instead nlmsg_put

[Jason Gunthorpe]

On Mon, Nov 07, 2022 at 10:51:33AM +0200, Leon Romanovsky wrote:
> From: Or Har-Toov <ohartoov@nvidia.com>
> 
> Using nlmsg_put causes static analysis tools to many
> false positives of not checking the return value of nlmsg_put.
> 
> In all uses in nldev.c, payload parameter is 0 so NULL will never
> be returned. So let's use __nlmsg_put function to silence the
> warnings.

I'd rather just add useless checks for the errors than call a private
function like this. Or add some nlmsg_put_no_payload() that can't fail

Jason

Re: [PATCH rdma-next 1/4] RDMA/nldev: Use __nlmsg_put instead nlmsg_put

[Leon Romanovsky]

On Wed, Nov 09, 2022 at 03:15:30PM -0400, Jason Gunthorpe wrote:
> On Mon, Nov 07, 2022 at 10:51:33AM +0200, Leon Romanovsky wrote:
> > From: Or Har-Toov <ohartoov@nvidia.com>
> > 
> > Using nlmsg_put causes static analysis tools to many
> > false positives of not checking the return value of nlmsg_put.
> > 
> > In all uses in nldev.c, payload parameter is 0 so NULL will never
> > be returned. So let's use __nlmsg_put function to silence the
> > warnings.
> 
> I'd rather just add useless checks for the errors than call a private
> function like this. Or add some nlmsg_put_no_payload() that can't fail

This is exactly what __nlmsg_put() means. Function that can't fail.

Thanks

> 
> Jason

Re: [PATCH rdma-next 1/4] RDMA/nldev: Use __nlmsg_put instead nlmsg_put

[Jason Gunthorpe]

On Sun, Nov 13, 2022 at 09:19:59AM +0200, Leon Romanovsky wrote:
> On Wed, Nov 09, 2022 at 03:15:30PM -0400, Jason Gunthorpe wrote:
> > On Mon, Nov 07, 2022 at 10:51:33AM +0200, Leon Romanovsky wrote:
> > > From: Or Har-Toov <ohartoov@nvidia.com>
> > > 
> > > Using nlmsg_put causes static analysis tools to many
> > > false positives of not checking the return value of nlmsg_put.
> > > 
> > > In all uses in nldev.c, payload parameter is 0 so NULL will never
> > > be returned. So let's use __nlmsg_put function to silence the
> > > warnings.
> > 
> > I'd rather just add useless checks for the errors than call a private
> > function like this. Or add some nlmsg_put_no_payload() that can't fail
> 
> This is exactly what __nlmsg_put() means. Function that can't fail.

Er no, it is some internal function. A function that can't fail
wouldn't accept the payload argument at all.

Jason

Re: [PATCH rdma-next 1/4] RDMA/nldev: Use __nlmsg_put instead nlmsg_put

[Leon Romanovsky]

On Mon, Nov 14, 2022 at 10:17:12AM -0400, Jason Gunthorpe wrote:
> On Sun, Nov 13, 2022 at 09:19:59AM +0200, Leon Romanovsky wrote:
> > On Wed, Nov 09, 2022 at 03:15:30PM -0400, Jason Gunthorpe wrote:
> > > On Mon, Nov 07, 2022 at 10:51:33AM +0200, Leon Romanovsky wrote:
> > > > From: Or Har-Toov <ohartoov@nvidia.com>
> > > > 
> > > > Using nlmsg_put causes static analysis tools to many
> > > > false positives of not checking the return value of nlmsg_put.
> > > > 
> > > > In all uses in nldev.c, payload parameter is 0 so NULL will never
> > > > be returned. So let's use __nlmsg_put function to silence the
> > > > warnings.
> > > 
> > > I'd rather just add useless checks for the errors than call a private
> > > function like this. Or add some nlmsg_put_no_payload() that can't fail
> > 
> > This is exactly what __nlmsg_put() means. Function that can't fail.
> 
> Er no, it is some internal function. A function that can't fail
> wouldn't accept the payload argument at all.

It is not internal to me, all users of __nlmsg_put() are outside of
netdev world.

Thanks

> 
> Jason

[PATCH rdma-next 2/4] RDMA/restrack: Release MR restrack when delete

[Leon Romanovsky]

From: Mark Zhang <markzhang@nvidia.com>

The MR restrack also needs to be released when delete it, otherwise it
cause memory leak as the task struct won't be released.

Fixes: 13ef5539def7 ("RDMA/restrack: Count references to the verbs objects")
Signed-off-by: Mark Zhang <markzhang@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/restrack.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
index 1f935d9f6178..01a499a8b88d 100644
--- a/drivers/infiniband/core/restrack.c
+++ b/drivers/infiniband/core/restrack.c
@@ -343,8 +343,6 @@ void rdma_restrack_del(struct rdma_restrack_entry *res)
 	rt = &dev->res[res->type];
 
 	old = xa_erase(&rt->xa, res->id);
-	if (res->type == RDMA_RESTRACK_MR)
-		return;
 	WARN_ON(old != res);
 
 out:
-- 
2.38.1

Re: [PATCH rdma-next 2/4] RDMA/restrack: Release MR restrack when delete

[Jason Gunthorpe]

On Mon, Nov 07, 2022 at 10:51:34AM +0200, Leon Romanovsky wrote:
> From: Mark Zhang <markzhang@nvidia.com>
> 
> The MR restrack also needs to be released when delete it, otherwise it
> cause memory leak as the task struct won't be released.
> 
> Fixes: 13ef5539def7 ("RDMA/restrack: Count references to the verbs objects")
> Signed-off-by: Mark Zhang <markzhang@nvidia.com>
> Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/infiniband/core/restrack.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> index 1f935d9f6178..01a499a8b88d 100644
> --- a/drivers/infiniband/core/restrack.c
> +++ b/drivers/infiniband/core/restrack.c
> @@ -343,8 +343,6 @@ void rdma_restrack_del(struct rdma_restrack_entry *res)
>  	rt = &dev->res[res->type];
>  
>  	old = xa_erase(&rt->xa, res->id);
> -	if (res->type == RDMA_RESTRACK_MR)
> -		return;

This needs more explanation, there was some good reason we needed to
avoid the wait_for_completion() for the driver allocated objects, but I
can't remember it anymore.

You added this code in the v2 of the original series, maybe it had
something to do with mlx4?

Jason

Re: [PATCH rdma-next 2/4] RDMA/restrack: Release MR restrack when delete

[Leon Romanovsky]

On Wed, Nov 09, 2022 at 03:08:33PM -0400, Jason Gunthorpe wrote:
> On Mon, Nov 07, 2022 at 10:51:34AM +0200, Leon Romanovsky wrote:
> > From: Mark Zhang <markzhang@nvidia.com>
> > 
> > The MR restrack also needs to be released when delete it, otherwise it
> > cause memory leak as the task struct won't be released.
> > 
> > Fixes: 13ef5539def7 ("RDMA/restrack: Count references to the verbs objects")
> > Signed-off-by: Mark Zhang <markzhang@nvidia.com>
> > Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  drivers/infiniband/core/restrack.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> > index 1f935d9f6178..01a499a8b88d 100644
> > --- a/drivers/infiniband/core/restrack.c
> > +++ b/drivers/infiniband/core/restrack.c
> > @@ -343,8 +343,6 @@ void rdma_restrack_del(struct rdma_restrack_entry *res)
> >  	rt = &dev->res[res->type];
> >  
> >  	old = xa_erase(&rt->xa, res->id);
> > -	if (res->type == RDMA_RESTRACK_MR)
> > -		return;
> 
> This needs more explanation, there was some good reason we needed to
> avoid the wait_for_completion() for the driver allocated objects, but I
> can't remember it anymore.
> 
> You added this code in the v2 of the original series, maybe it had
> something to do with mlx4?

I failed to remember either, but if you want even more magic in your life,
see this hilarious thread:
https://lore.kernel.org/linux-rdma/9ba5a611ceac86774d3d0fda12704cecc30606f9.1618753038.git.leonro@nvidia.com/

I kept this patch in my queue ~1 month and didn't get any complains.

Thanks

> 
> Jason

Re: [PATCH rdma-next 2/4] RDMA/restrack: Release MR restrack when delete

[Jason Gunthorpe]

On Thu, Nov 10, 2022 at 11:35:37AM +0200, Leon Romanovsky wrote:
> On Wed, Nov 09, 2022 at 03:08:33PM -0400, Jason Gunthorpe wrote:
> > On Mon, Nov 07, 2022 at 10:51:34AM +0200, Leon Romanovsky wrote:
> > > From: Mark Zhang <markzhang@nvidia.com>
> > > 
> > > The MR restrack also needs to be released when delete it, otherwise it
> > > cause memory leak as the task struct won't be released.
> > > 
> > > Fixes: 13ef5539def7 ("RDMA/restrack: Count references to the verbs objects")
> > > Signed-off-by: Mark Zhang <markzhang@nvidia.com>
> > > Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > > ---
> > >  drivers/infiniband/core/restrack.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> > > index 1f935d9f6178..01a499a8b88d 100644
> > > --- a/drivers/infiniband/core/restrack.c
> > > +++ b/drivers/infiniband/core/restrack.c
> > > @@ -343,8 +343,6 @@ void rdma_restrack_del(struct rdma_restrack_entry *res)
> > >  	rt = &dev->res[res->type];
> > >  
> > >  	old = xa_erase(&rt->xa, res->id);
> > > -	if (res->type == RDMA_RESTRACK_MR)
> > > -		return;
> > 
> > This needs more explanation, there was some good reason we needed to
> > avoid the wait_for_completion() for the driver allocated objects, but I
> > can't remember it anymore.
> > 
> > You added this code in the v2 of the original series, maybe it had
> > something to do with mlx4?
> 
> I failed to remember either, but if you want even more magic in your life,
> see this hilarious thread:
> https://lore.kernel.org/linux-rdma/9ba5a611ceac86774d3d0fda12704cecc30606f9.1618753038.git.leonro@nvidia.com/

Oh, that clears it up

The issue is that dereg can fail for MR:

	rdma_restrack_del(&mr->res);
	ret = mr->device->ops.dereg_mr(mr, udata);
	if (!ret) {
		atomic_dec(&pd->usecnt);

Because the driver management of the object puts it in the wrong
order.

The above if is necessary because if we trigger this failure path
without it, then the next attempt to free the MR will trigger a
WARN_ON.

So, no, this can't just be deleted, and the suggestions I gave in that
prior thread still look like they hold up. As would converting the mr
into core ownership. We are very close now, the mlx5 cache mess is
almost cleaned up enough to do it. Perhaps after Michael's series

Jason

Re: [PATCH rdma-next 2/4] RDMA/restrack: Release MR restrack when delete

[Leon Romanovsky]

On Thu, Nov 10, 2022 at 03:29:41PM -0400, Jason Gunthorpe wrote:
> On Thu, Nov 10, 2022 at 11:35:37AM +0200, Leon Romanovsky wrote:
> > On Wed, Nov 09, 2022 at 03:08:33PM -0400, Jason Gunthorpe wrote:
> > > On Mon, Nov 07, 2022 at 10:51:34AM +0200, Leon Romanovsky wrote:
> > > > From: Mark Zhang <markzhang@nvidia.com>
> > > > 
> > > > The MR restrack also needs to be released when delete it, otherwise it
> > > > cause memory leak as the task struct won't be released.
> > > > 
> > > > Fixes: 13ef5539def7 ("RDMA/restrack: Count references to the verbs objects")
> > > > Signed-off-by: Mark Zhang <markzhang@nvidia.com>
> > > > Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
> > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > > > ---
> > > >  drivers/infiniband/core/restrack.c | 2 --
> > > >  1 file changed, 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> > > > index 1f935d9f6178..01a499a8b88d 100644
> > > > --- a/drivers/infiniband/core/restrack.c
> > > > +++ b/drivers/infiniband/core/restrack.c
> > > > @@ -343,8 +343,6 @@ void rdma_restrack_del(struct rdma_restrack_entry *res)
> > > >  	rt = &dev->res[res->type];
> > > >  
> > > >  	old = xa_erase(&rt->xa, res->id);
> > > > -	if (res->type == RDMA_RESTRACK_MR)
> > > > -		return;
> > > 
> > > This needs more explanation, there was some good reason we needed to
> > > avoid the wait_for_completion() for the driver allocated objects, but I
> > > can't remember it anymore.
> > > 
> > > You added this code in the v2 of the original series, maybe it had
> > > something to do with mlx4?
> > 
> > I failed to remember either, but if you want even more magic in your life,
> > see this hilarious thread:
> > https://lore.kernel.org/linux-rdma/9ba5a611ceac86774d3d0fda12704cecc30606f9.1618753038.git.leonro@nvidia.com/
> 
> Oh, that clears it up
> 
> The issue is that dereg can fail for MR:
> 
> 	rdma_restrack_del(&mr->res);
> 	ret = mr->device->ops.dereg_mr(mr, udata);
> 	if (!ret) {
> 		atomic_dec(&pd->usecnt);
> 
> Because the driver management of the object puts it in the wrong
> order.
> 
> The above if is necessary because if we trigger this failure path
> without it, then the next attempt to free the MR will trigger a
> WARN_ON.

Not really, after first entry to rdma_restrack_del(), we will set
res->valid to false. Any subsequent calls to rdma_restrack_del() will
do nothing.

  322 void rdma_restrack_del(struct rdma_restrack_entry *res)
  323 {
  324         struct rdma_restrack_entry *old;
  325         struct rdma_restrack_root *rt;
  326         struct ib_device *dev;
  327
  328         if (!res->valid) {
  329                 if (res->task) {
  330                         put_task_struct(res->task);
  331                         res->task = NULL;
  332                 }
  333                 return; <------- exit
  334         }

Thanks

Re: [PATCH rdma-next 2/4] RDMA/restrack: Release MR restrack when delete

[Jason Gunthorpe]

On Thu, Nov 10, 2022 at 09:39:51PM +0200, Leon Romanovsky wrote:
> On Thu, Nov 10, 2022 at 03:29:41PM -0400, Jason Gunthorpe wrote:
> > On Thu, Nov 10, 2022 at 11:35:37AM +0200, Leon Romanovsky wrote:
> > > On Wed, Nov 09, 2022 at 03:08:33PM -0400, Jason Gunthorpe wrote:
> > > > On Mon, Nov 07, 2022 at 10:51:34AM +0200, Leon Romanovsky wrote:
> > > > > From: Mark Zhang <markzhang@nvidia.com>
> > > > > 
> > > > > The MR restrack also needs to be released when delete it, otherwise it
> > > > > cause memory leak as the task struct won't be released.
> > > > > 
> > > > > Fixes: 13ef5539def7 ("RDMA/restrack: Count references to the verbs objects")
> > > > > Signed-off-by: Mark Zhang <markzhang@nvidia.com>
> > > > > Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
> > > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > > > > ---
> > > > >  drivers/infiniband/core/restrack.c | 2 --
> > > > >  1 file changed, 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> > > > > index 1f935d9f6178..01a499a8b88d 100644
> > > > > --- a/drivers/infiniband/core/restrack.c
> > > > > +++ b/drivers/infiniband/core/restrack.c
> > > > > @@ -343,8 +343,6 @@ void rdma_restrack_del(struct rdma_restrack_entry *res)
> > > > >  	rt = &dev->res[res->type];
> > > > >  
> > > > >  	old = xa_erase(&rt->xa, res->id);
> > > > > -	if (res->type == RDMA_RESTRACK_MR)
> > > > > -		return;
> > > > 
> > > > This needs more explanation, there was some good reason we needed to
> > > > avoid the wait_for_completion() for the driver allocated objects, but I
> > > > can't remember it anymore.
> > > > 
> > > > You added this code in the v2 of the original series, maybe it had
> > > > something to do with mlx4?
> > > 
> > > I failed to remember either, but if you want even more magic in your life,
> > > see this hilarious thread:
> > > https://lore.kernel.org/linux-rdma/9ba5a611ceac86774d3d0fda12704cecc30606f9.1618753038.git.leonro@nvidia.com/
> > 
> > Oh, that clears it up
> > 
> > The issue is that dereg can fail for MR:
> > 
> > 	rdma_restrack_del(&mr->res);
> > 	ret = mr->device->ops.dereg_mr(mr, udata);
> > 	if (!ret) {
> > 		atomic_dec(&pd->usecnt);
> > 
> > Because the driver management of the object puts it in the wrong
> > order.
> > 
> > The above if is necessary because if we trigger this failure path
> > without it, then the next attempt to free the MR will trigger a
> > WARN_ON.
> 
> Not really, after first entry to rdma_restrack_del(), we will set
> res->valid to false. Any subsequent calls to rdma_restrack_del() will
> do nothing.

Uh, that does seem OK

So I'm back to I don't know why this if was put in.

Jason

[PATCH rdma-next 3/4] RDMA/core: Make sure "ib_port" is valid when access sysfs node

[Leon Romanovsky]

From: Mark Zhang <markzhang@nvidia.com>

The "ib_port" structure must be set before adding the sysfs kobject,
and reset after removing it, otherwise it may crash when accessing
the sysfs node:
  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000050
  Mem abort info:
    ESR = 0x96000006
    Exception class = DABT (current EL), IL = 32 bits
    SET = 0, FnV = 0
    EA = 0, S1PTW = 0
  Data abort info:
    ISV = 0, ISS = 0x00000006
    CM = 0, WnR = 0
  user pgtable: 4k pages, 48-bit VAs, pgdp = 00000000e85f5ba5
  [0000000000000050] pgd=0000000848fd9003, pud=000000085b387003, pmd=0000000000000000
  Internal error: Oops: 96000006 [#2] PREEMPT SMP
  Modules linked in: ib_umad(O) mlx5_ib(O) nfnetlink_cttimeout(E) nfnetlink(E) act_gact(E) cls_flower(E) sch_ingress(E) openvswitch(E) nsh(E) nf_nat_ipv6(E) nf_nat_ipv4(E) nf_conncount(E) nf_nat(E) nf_conntrack(E) nf_defrag_ipv6(E) nf_defrag_ipv4(E) mst_pciconf(O) ipmi_devintf(E) ipmi_msghandler(E) ipmb_dev_int(OE) mlx5_core(O) mlxfw(O) mlxdevm(O) auxiliary(O) ib_uverbs(O) ib_core(O) mlx_compat(O) psample(E) sbsa_gwdt(E) uio_pdrv_genirq(E) uio(E) mlxbf_pmc(OE) mlxbf_gige(OE) mlxbf_tmfifo(OE) gpio_mlxbf2(OE) pwr_mlxbf(OE) mlx_trio(OE) i2c_mlxbf(OE) mlx_bootctl(OE) bluefield_edac(OE) knem(O) ip_tables(E) ipv6(E) crc_ccitt(E) [last unloaded: mst_pci]
  Process grep (pid: 3372, stack limit = 0x0000000022055c92)
  CPU: 5 PID: 3372 Comm: grep Tainted: G      D    OE     4.19.161-mlnx.47.gadcd9e3 #1
  Hardware name: https://www.mellanox.com BlueField SoC/BlueField SoC, BIOS BlueField:3.9.2-15-ga2403ab Sep  8 2022
  pstate: 40000005 (nZcv daif -PAN -UAO)
  pc : hw_stat_port_show+0x4c/0x80 [ib_core]
  lr : port_attr_show+0x40/0x58 [ib_core]
  sp : ffff000029f43b50
  x29: ffff000029f43b50 x28: 0000000019375000
  x27: ffff8007b821a540 x26: ffff000029f43e30
  x25: 0000000000008000 x24: ffff000000eaa958
  x23: 0000000000001000 x22: ffff8007a4ce3000
  x21: ffff8007baff8000 x20: ffff8007b9066ac0
  x19: ffff8007bae97578 x18: 0000000000000000
  x17: 0000000000000000 x16: 0000000000000000
  x15: 0000000000000000 x14: 0000000000000000
  x13: 0000000000000000 x12: 0000000000000000
  x11: 0000000000000000 x10: 0000000000000000
  x9 : 0000000000000000 x8 : ffff8007a4ce4000
  x7 : 0000000000000000 x6 : 000000000000003f
  x5 : ffff000000e6a280 x4 : ffff8007a4ce3000
  x3 : 0000000000000000 x2 : aaaaaaaaaaaaaaab
  x1 : ffff8007b9066a10 x0 : ffff8007baff8000
  Call trace:
   hw_stat_port_show+0x4c/0x80 [ib_core]
   port_attr_show+0x40/0x58 [ib_core]
   sysfs_kf_seq_show+0x8c/0x150
   kernfs_seq_show+0x44/0x50
   seq_read+0x1b4/0x45c
   kernfs_fop_read+0x148/0x1d8
   __vfs_read+0x58/0x180
   vfs_read+0x94/0x154
   ksys_read+0x68/0xd8
   __arm64_sys_read+0x28/0x34
   el0_svc_common+0x88/0x18c
   el0_svc_handler+0x78/0x94
   el0_svc+0x8/0xe8
  Code: f2955562 aa1603e4 aa1503e0 f9405683 (f9402861)

Fixes: d8a5883814b9 ("RDMA/core: Replace the ib_port_data hw_stats pointers with a ib_port pointer")
Signed-off-by: Mark Zhang <markzhang@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/sysfs.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
index 84c53bd2a52d..ee59d7391568 100644
--- a/drivers/infiniband/core/sysfs.c
+++ b/drivers/infiniband/core/sysfs.c
@@ -1213,6 +1213,9 @@ static struct ib_port *setup_port(struct ib_core_device *coredev, int port_num,
 	p->port_num = port_num;
 	kobject_init(&p->kobj, &port_type);
 
+	if (device->port_data && is_full_dev)
+		device->port_data[port_num].sysfs = p;
+
 	cur_group = p->groups_list;
 	ret = alloc_port_table_group("gids", &p->groups[0], p->attrs_list,
 				     attr->gid_tbl_len, show_port_gid);
@@ -1258,9 +1261,6 @@ static struct ib_port *setup_port(struct ib_core_device *coredev, int port_num,
 	}
 
 	list_add_tail(&p->kobj.entry, &coredev->port_list);
-	if (device->port_data && is_full_dev)
-		device->port_data[port_num].sysfs = p;
-
 	return p;
 
 err_groups:
@@ -1268,6 +1268,8 @@ static struct ib_port *setup_port(struct ib_core_device *coredev, int port_num,
 err_del:
 	kobject_del(&p->kobj);
 err_put:
+	if (device->port_data && is_full_dev)
+		device->port_data[port_num].sysfs = NULL;
 	kobject_put(&p->kobj);
 	return ERR_PTR(ret);
 }
@@ -1276,14 +1278,17 @@ static void destroy_port(struct ib_core_device *coredev, struct ib_port *port)
 {
 	bool is_full_dev = &port->ibdev->coredev == coredev;
 
-	if (port->ibdev->port_data &&
-	    port->ibdev->port_data[port->port_num].sysfs == port)
-		port->ibdev->port_data[port->port_num].sysfs = NULL;
 	list_del(&port->kobj.entry);
 	if (is_full_dev)
 		sysfs_remove_groups(&port->kobj, port->ibdev->ops.port_groups);
+
 	sysfs_remove_groups(&port->kobj, port->groups_list);
 	kobject_del(&port->kobj);
+
+	if (port->ibdev->port_data &&
+	    port->ibdev->port_data[port->port_num].sysfs == port)
+		port->ibdev->port_data[port->port_num].sysfs = NULL;
+
 	kobject_put(&port->kobj);
 }
 
-- 
2.38.1

[PATCH rdma-next 4/4] RDMA/nldev: Return "-EAGAIN" if the cm_id isn't from expected port

[Leon Romanovsky]

From: Mark Zhang <markzhang@nvidia.com>

When filling a cm_id entry, return "-EAGAIN" instead of 0 if the cm_id
doesn'the have the same port as requested, otherwise an incomplete entry
may be returned, which causes "rdam res show cm_id" to return an error.

For example on a machine with two rdma devices with "rping -C 1 -v -s"
running background, the "rdma" command fails:
  $ rdma -V
  rdma utility, iproute2-5.19.0
  $ rdma res show cm_id
  link mlx5_0/- cm-idn 0 state LISTEN ps TCP pid 28056 comm rping src-addr 0.0.0.0:7174
  error: Protocol not available

While with this fix it succeeds:
  $ rdma res show cm_id
  link mlx5_0/- cm-idn 0 state LISTEN ps TCP pid 26395 comm rping src-addr 0.0.0.0:7174
  link mlx5_1/- cm-idn 0 state LISTEN ps TCP pid 26395 comm rping src-addr 0.0.0.0:7174

Fixes: 00313983cda6 ("RDMA/nldev: provide detailed CM_ID information")
Signed-off-by: Mark Zhang <markzhang@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/nldev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
index 213be4e6fd28..cfe569fefbc4 100644
--- a/drivers/infiniband/core/nldev.c
+++ b/drivers/infiniband/core/nldev.c
@@ -552,7 +552,7 @@ static int fill_res_cm_id_entry(struct sk_buff *msg, bool has_cap_net_admin,
 	struct rdma_cm_id *cm_id = &id_priv->id;
 
 	if (port && port != cm_id->port_num)
-		return 0;
+		return -EAGAIN;
 
 	if (cm_id->port_num &&
 	    nla_put_u32(msg, RDMA_NLDEV_ATTR_PORT_INDEX, cm_id->port_num))
-- 
2.38.1

Re: [PATCH rdma-next 0/4] Various core fixes

[Leon Romanovsky]

On Mon, Nov 07, 2022 at 10:51:32AM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Batch unrelated fixes.
> 
> Mark Zhang (3):
>   RDMA/restrack: Release MR restrack when delete
>   RDMA/core: Make sure "ib_port" is valid when access sysfs node
>   RDMA/nldev: Return "-EAGAIN" if the cm_id isn't from expected port

I took these three.

> 
> Or Har-Toov (1):
>   RDMA/nldev: Use __nlmsg_put instead nlmsg_put

This is not ready yet.

Thanks

> 
>  drivers/infiniband/core/nldev.c    | 110 ++++++++++++++---------------
>  drivers/infiniband/core/restrack.c |   2 -
>  drivers/infiniband/core/sysfs.c    |  17 +++--
>  3 files changed, 64 insertions(+), 65 deletions(-)
> 
> -- 
> 2.38.1
> 

Re: (subset) [PATCH rdma-next 0/4] Various core fixes

[Leon Romanovsky]

On Mon, 7 Nov 2022 10:51:32 +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Batch unrelated fixes.
> 
> Mark Zhang (3):
>   RDMA/restrack: Release MR restrack when delete
>   RDMA/core: Make sure "ib_port" is valid when access sysfs node
>   RDMA/nldev: Return "-EAGAIN" if the cm_id isn't from expected port
> 
> [...]

Applied, thanks!

[2/4] RDMA/restrack: Release MR restrack when delete
      https://git.kernel.org/rdma/rdma/c/dac153f2802db1
[3/4] RDMA/core: Make sure "ib_port" is valid when access sysfs node
      https://git.kernel.org/rdma/rdma/c/5e15ff29b156bb
[4/4] RDMA/nldev: Return "-EAGAIN" if the cm_id isn't from expected port
      https://git.kernel.org/rdma/rdma/c/ecacb3751f2545

Best regards,
-- 
Leon Romanovsky <leon@kernel.org>