| Message ID | 20191216120436.3204814-1-haakon.bugge@oracle.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | a242c36951ecd24bc16086940dbe6b522205c461 |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [RDMA/netlink,v2] RDMA/netlink: Adhere to returning zero on success | expand |
On Mon, Dec 16, 2019 at 01:04:36PM +0100, Håkon Bugge wrote: > In rdma_nl_rcv_skb(), the local variable err is assigned the return > value of the supplied callback function, which could be one of > ib_nl_handle_resolve_resp(), ib_nl_handle_set_timeout(), or > ib_nl_handle_ip_res_resp(). These three functions all return skb->len > on success. > > rdma_nl_rcv_skb() is merely a copy of netlink_rcv_skb(). The callback > functions used by the latter have the convention: "Returns 0 on > success or a negative error code". > > In particular, the statement (equal for both functions): > > if (nlh->nlmsg_flags & NLM_F_ACK || err) > > implies that rdma_nl_rcv_skb() always will ack a message, independent > of the NLM_F_ACK being set in nlmsg_flags or not. > > The fix could be to change the above statement, but it is better to > keep the two *_rcv_skb() functions equal in this respect and instead > change the callback functions in the rdma subsystem to the correct > convention. > > Suggested-by: Mark Haywood <mark.haywood@oracle.com> > Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> > Tested-by: Mark Haywood <mark.haywood@oracle.com> > Fixes: 2ca546b92a02 ("IB/sa: Route SA pathrecord query through netlink") > Fixes: ae43f8286730 ("IB/core: Add IP to GID netlink offload") > > --- > v1 -> v2: > * Realized sk_buff::len is unsigned, hence simply returning > zero in the good case > --- > drivers/infiniband/core/addr.c | 2 +- > drivers/infiniband/core/sa_query.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > Better to put Fixes above *-by tags. Thanks, Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On Mon, Dec 16, 2019 at 01:04:36PM +0100, Håkon Bugge wrote: > In rdma_nl_rcv_skb(), the local variable err is assigned the return > value of the supplied callback function, which could be one of > ib_nl_handle_resolve_resp(), ib_nl_handle_set_timeout(), or > ib_nl_handle_ip_res_resp(). These three functions all return skb->len > on success. > > rdma_nl_rcv_skb() is merely a copy of netlink_rcv_skb(). The callback > functions used by the latter have the convention: "Returns 0 on > success or a negative error code". > > In particular, the statement (equal for both functions): > > if (nlh->nlmsg_flags & NLM_F_ACK || err) > > implies that rdma_nl_rcv_skb() always will ack a message, independent > of the NLM_F_ACK being set in nlmsg_flags or not. > > The fix could be to change the above statement, but it is better to > keep the two *_rcv_skb() functions equal in this respect and instead > change the callback functions in the rdma subsystem to the correct > convention. > > Suggested-by: Mark Haywood <mark.haywood@oracle.com> > Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> > Tested-by: Mark Haywood <mark.haywood@oracle.com> > Fixes: 2ca546b92a02 ("IB/sa: Route SA pathrecord query through netlink") > Fixes: ae43f8286730 ("IB/core: Add IP to GID netlink offload") > --- > v1 -> v2: > * Realized sk_buff::len is unsigned, hence simply returning > zero in the good case > --- > drivers/infiniband/core/addr.c | 2 +- > drivers/infiniband/core/sa_query.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) Applied to for-next, thanks Jason
diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c index 606fa6d86685..1753a9801b70 100644 --- a/drivers/infiniband/core/addr.c +++ b/drivers/infiniband/core/addr.c @@ -139,7 +139,7 @@ int ib_nl_handle_ip_res_resp(struct sk_buff *skb, if (ib_nl_is_good_ip_resp(nlh)) ib_nl_process_good_ip_rsep(nlh); - return skb->len; + return 0; } static int ib_nl_ip_send_msg(struct rdma_dev_addr *dev_addr, diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c index 8917125ea16d..30d4c126a2db 100644 --- a/drivers/infiniband/core/sa_query.c +++ b/drivers/infiniband/core/sa_query.c @@ -1068,7 +1068,7 @@ int ib_nl_handle_set_timeout(struct sk_buff *skb, } settimeout_out: - return skb->len; + return 0; } static inline int ib_nl_is_good_resolve_resp(const struct nlmsghdr *nlh) @@ -1139,7 +1139,7 @@ int ib_nl_handle_resolve_resp(struct sk_buff *skb, } resp_out: - return skb->len; + return 0; } static void free_sm_ah(struct kref *kref)