diff mbox series

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

Message ID 3d8fb9edbd41f122fda680158a80bac44e55e847.1667810736.git.leonro@nvidia.com (mailing list archive)
State Changes Requested
Headers show
Series Various core fixes | expand

Commit Message

Leon Romanovsky Nov. 7, 2022, 8:51 a.m. UTC 
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(-)

Comments

Jason Gunthorpe Nov. 9, 2022, 7:15 p.m. UTC | #1 
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
Leon Romanovsky Nov. 13, 2022, 7:19 a.m. UTC | #2 
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
Jason Gunthorpe Nov. 14, 2022, 2:17 p.m. UTC | #3 
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
Leon Romanovsky Nov. 15, 2022, 7:52 a.m. UTC | #4 
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
diff mbox series

Patch

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) ||