> On 11 Dec 2019, at 11:34, Håkon Bugge <haakon.bugge@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Håkon Bugge <haakon.bugge@xxxxxxxxxx>
> Tested-by: Mark Haywood <mark.haywood@xxxxxxxxxx>
If accepted, please add:
Fixes: 2ca546b92a02 ("IB/sa: Route SA pathrecord query through netlink")
Fixes: ae43f8286730 ("IB/core: Add IP to GID netlink offload")
or give me a hint to send a v2.
Thxs, Håkon
> ---
> drivers/infiniband/core/addr.c | 2 +-
> drivers/infiniband/core/sa_query.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
> index 606fa6d86685..9449ed2536fa 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 skb->len > 0 ? 0 : skb->len;
> }
>
> 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..dc249e382367 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 skb->len > 0 ? 0 : skb->len;
> }
>
> 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 skb->len > 0 ? 0 : skb->len;
> }
>
> static void free_sm_ah(struct kref *kref)
> --
> 2.20.1
>
