[RDMA/netlink] RDMA/netlink: Adhere to returning zero on success

Commit Message

Haakon Bugge Dec. 11, 2019, 10:34 a.m. UTC

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 <[e-mail]>
Signed-off-by: Håkon Bugge <[e-mail]>
Tested-by: Mark Haywood <[e-mail]>
---
 drivers/infiniband/core/addr.c     | 2 +-
 drivers/infiniband/core/sa_query.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Haakon Bugge Dec. 11, 2019, 10:42 a.m. UTC | #1

> On 11 Dec 2019, at 11:34, Håkon Bugge <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Håkon Bugge <[e-mail]>
> Tested-by: Mark Haywood <[e-mail]>

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
>

Leon Romanovsky Dec. 11, 2019, 12:39 p.m. UTC | #2

On Wed, Dec 11, 2019 at 11:34:00AM +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 more accurate description is that rdma_nl_rcv_skb() always generates
NLMSG_ERROR without relation to NLM_F_ACK flag. The NLM_F_ACK flag is
requested to get acknowledges for the success.

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

AFAIR, RTNETLINK has the same implementation as RDMA netlink.

Thanks

Haakon Bugge Dec. 11, 2019, 1:13 p.m. UTC | #3

> On 11 Dec 2019, at 13:39, Leon Romanovsky <[e-mail]> wrote:
> 
> On Wed, Dec 11, 2019 at 11:34:00AM +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 more accurate description is that rdma_nl_rcv_skb() always generates
> NLMSG_ERROR without relation to NLM_F_ACK flag. The NLM_F_ACK flag is
> requested to get acknowledges for the success.
> 
>> 
>> 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.
> 
> AFAIR, RTNETLINK has the same implementation as RDMA netlink.

With the exception of the callback functions, as noted above.


Thxs, Håkon

> 
> Thanks

Haakon Bugge Dec. 11, 2019, 7:31 p.m. UTC | #4

> On 11 Dec 2019, at 14:13, Håkon Bugge <[e-mail]> wrote:
> 
> 
> 
>> On 11 Dec 2019, at 13:39, Leon Romanovsky <[e-mail]> wrote:
>> 
>> On Wed, Dec 11, 2019 at 11:34:00AM +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 more accurate description is that rdma_nl_rcv_skb() always generates
>> NLMSG_ERROR without relation to NLM_F_ACK flag. The NLM_F_ACK flag is
>> requested to get acknowledges for the success.


Yes. And when, lets say a legitimate path record response, containing N positive bytes, is sent back from ibacm to the kernel, rdma_nl_rcv_skb() think this is an error, due to "if (nlh->nlmsg_flags & NLM_F_ACK || err)" _and_ ib_nl_handle_resolve_resp() returning N.

Thxs, Håkon


>> 
>>> 
>>> 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.
>> 
>> AFAIR, RTNETLINK has the same implementation as RDMA netlink.
> 
> With the exception of the callback functions, as noted above.
> 
> 
> Thxs, Håkon
> 
>> 
>> Thanks
>

Leon Romanovsky Dec. 12, 2019, 11:40 a.m. UTC | #5

On Wed, Dec 11, 2019 at 08:31:18PM +0100, Håkon Bugge wrote:
>
>
> > On 11 Dec 2019, at 14:13, Håkon Bugge <[e-mail]> wrote:
> >
> >
> >
> >> On 11 Dec 2019, at 13:39, Leon Romanovsky <[e-mail]> wrote:
> >>
> >> On Wed, Dec 11, 2019 at 11:34:00AM +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 more accurate description is that rdma_nl_rcv_skb() always generates
> >> NLMSG_ERROR without relation to NLM_F_ACK flag. The NLM_F_ACK flag is
> >> requested to get acknowledges for the success.
>
>
> Yes. And when, lets say a legitimate path record response, containing N positive bytes, is sent back from ibacm to the kernel, rdma_nl_rcv_skb() think this is an error, due to "if (nlh->nlmsg_flags & NLM_F_ACK || err)" _and_ ib_nl_handle_resolve_resp() returning N.

How did you test this patch?
Do we have open-source applications which don't set NLM_F_ACK for
ib_nl_*() calls?

Thanks

>
> Thxs, Håkon
>
>
> >>
> >>>
> >>> 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.
> >>
> >> AFAIR, RTNETLINK has the same implementation as RDMA netlink.
> >
> > With the exception of the callback functions, as noted above.
> >
> >
> > Thxs, Håkon
> >
> >>
> >> Thanks
> >
>

Haakon Bugge Dec. 12, 2019, 12:22 p.m. UTC | #6

Missed the reply-all somewhere in this exchange.

-h


> On 12 Dec 2019, at 13:16, Håkon Bugge <[e-mail]> wrote:
> 
> 
> 
>> On 12 Dec 2019, at 13:10, Leon Romanovsky <[e-mail]> wrote:
>> 
>> On Thu, Dec 12, 2019 at 12:59:51PM +0100, Håkon Bugge wrote:
>>> 
>>> 
>>>> On 12 Dec 2019, at 12:40, Leon Romanovsky <[e-mail]> wrote:
>>>> 
>>>> On Wed, Dec 11, 2019 at 08:31:18PM +0100, Håkon Bugge wrote:
>>>>> 
>>>>> 
>>>>>> On 11 Dec 2019, at 14:13, Håkon Bugge <[e-mail]> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>> On 11 Dec 2019, at 13:39, Leon Romanovsky <[e-mail]> wrote:
>>>>>>> 
>>>>>>> On Wed, Dec 11, 2019 at 11:34:00AM +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 more accurate description is that rdma_nl_rcv_skb() always generates
>>>>>>> NLMSG_ERROR without relation to NLM_F_ACK flag. The NLM_F_ACK flag is
>>>>>>> requested to get acknowledges for the success.
>>>>> 
>>>>> 
>>>>> Yes. And when, lets say a legitimate path record response, containing N positive bytes, is sent back from ibacm to the kernel, rdma_nl_rcv_skb() think this is an error, due to "if (nlh->nlmsg_flags & NLM_F_ACK || err)" _and_ ib_nl_handle_resolve_resp() returning N.
>>>> 
>>>> How did you test this patch?
>>>> Do we have open-source applications which don't set NLM_F_ACK for
>>>> ib_nl_*() calls?
>>> 
>>> As I alluded to above, yes, ibacm doesn't set it.
>> 
>> In this regards, I'm amazed that this patch didn't break ibacm.
> 
> On the contrary. The patch avoids the kernel sending back an error/ACK for every path record / resolve response.
> 
> 
> Håkon
> 
>> 
>> Thanks
>

Haakon Bugge Dec. 12, 2019, 12:37 p.m. UTC | #7

> On 12 Dec 2019, at 13:27, Leon Romanovsky <[e-mail]> wrote:
> 
> On Thu, Dec 12, 2019 at 01:16:54PM +0100, Håkon Bugge wrote:
>> 
>> 
>>> On 12 Dec 2019, at 13:10, Leon Romanovsky <[e-mail]> wrote:
>>> 
>>> On Thu, Dec 12, 2019 at 12:59:51PM +0100, Håkon Bugge wrote:
>>>> 
>>>> 
>>>>> On 12 Dec 2019, at 12:40, Leon Romanovsky <[e-mail]> wrote:
>>>>> 
>>>>> On Wed, Dec 11, 2019 at 08:31:18PM +0100, Håkon Bugge wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 11 Dec 2019, at 14:13, Håkon Bugge <[e-mail]> wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>>> On 11 Dec 2019, at 13:39, Leon Romanovsky <[e-mail]> wrote:
>>>>>>>> 
>>>>>>>> On Wed, Dec 11, 2019 at 11:34:00AM +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 more accurate description is that rdma_nl_rcv_skb() always generates
>>>>>>>> NLMSG_ERROR without relation to NLM_F_ACK flag. The NLM_F_ACK flag is
>>>>>>>> requested to get acknowledges for the success.
>>>>>> 
>>>>>> 
>>>>>> Yes. And when, lets say a legitimate path record response, containing N positive bytes, is sent back from ibacm to the kernel, rdma_nl_rcv_skb() think this is an error, due to "if (nlh->nlmsg_flags & NLM_F_ACK || err)" _and_ ib_nl_handle_resolve_resp() returning N.
>>>>> 
>>>>> How did you test this patch?
>>>>> Do we have open-source applications which don't set NLM_F_ACK for
>>>>> ib_nl_*() calls?
>>>> 
>>>> As I alluded to above, yes, ibacm doesn't set it.
>>> 
>>> In this regards, I'm amazed that this patch didn't break ibacm.
>> 
>> On the contrary. The patch avoids the kernel sending back an error/ACK for every path record / resolve response.
> 
> As long as ibacm continues to work with this patch, i'm ok.
> What type of testing did you perform?

I'll let Mark respond to the testing. The background is that ibacm was very *liberal* when it comes checking the requests it received from the kernel. In an attempt to tighten that, Mark discovered that ibacm received an unexpected ACK from the kernel just after having sent a response.

That aside, I think the RDMA NL callbacks shall adhere to the RTNETLINK conventions, thus, that's why this commit changes the callbacks and not the  rdma_nl_rcv_skb().


Thxs, Håkon

> 
> Thanks
> 
>> 
>> 
>> Håkon
>> 
>>> 
>>> Thanks

Mark Haywood Dec. 12, 2019, 2:14 p.m. UTC | #8

On 12/12/19 7:37 AM, Håkon Bugge wrote:
>
>> On 12 Dec 2019, at 13:27, Leon Romanovsky <[e-mail]> wrote:
>>
>> On Thu, Dec 12, 2019 at 01:16:54PM +0100, Håkon Bugge wrote:
>>>
>>>> On 12 Dec 2019, at 13:10, Leon Romanovsky <[e-mail]> wrote:
>>>>
>>>> On Thu, Dec 12, 2019 at 12:59:51PM +0100, Håkon Bugge wrote:
>>>>>
>>>>>> On 12 Dec 2019, at 12:40, Leon Romanovsky <[e-mail]> wrote:
>>>>>>
>>>>>> On Wed, Dec 11, 2019 at 08:31:18PM +0100, Håkon Bugge wrote:
>>>>>>>
>>>>>>>> On 11 Dec 2019, at 14:13, Håkon Bugge <[e-mail]> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> On 11 Dec 2019, at 13:39, Leon Romanovsky <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> On Wed, Dec 11, 2019 at 11:34:00AM +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 more accurate description is that rdma_nl_rcv_skb() always generates
>>>>>>>>> NLMSG_ERROR without relation to NLM_F_ACK flag. The NLM_F_ACK flag is
>>>>>>>>> requested to get acknowledges for the success.
>>>>>>>
>>>>>>> Yes. And when, lets say a legitimate path record response, containing N positive bytes, is sent back from ibacm to the kernel, rdma_nl_rcv_skb() think this is an error, due to "if (nlh->nlmsg_flags & NLM_F_ACK || err)" _and_ ib_nl_handle_resolve_resp() returning N.
>>>>>> How did you test this patch?
>>>>>> Do we have open-source applications which don't set NLM_F_ACK for
>>>>>> ib_nl_*() calls?
>>>>> As I alluded to above, yes, ibacm doesn't set it.
>>>> In this regards, I'm amazed that this patch didn't break ibacm.
>>> On the contrary. The patch avoids the kernel sending back an error/ACK for every path record / resolve response.
>> As long as ibacm continues to work with this patch, i'm ok.
>> What type of testing did you perform?
> I'll let Mark respond to the testing. The background is that ibacm was very *liberal* when it comes checking the requests it received from the kernel. In an attempt to tighten that, Mark discovered that ibacm received an unexpected ACK from the kernel just after having sent a response.


Without this patch, for every response to a RDMA_NL_LS_OP_RESOLVE 
request, ibacm receives an ACK with a nlmsgerr error value equal to the 
length of the response message.

With this patch, no ACKs are received.

If I add the NLM_F_ACK to the nlmsg_header flags when responding to the 
RDMA_NL_LS_OP_RESOLVE request, ibacm once again receives the ACKs.

Mark


>
> That aside, I think the RDMA NL callbacks shall adhere to the RTNETLINK conventions, thus, that's why this commit changes the callbacks and not the  rdma_nl_rcv_skb().
>
>
> Thxs, Håkon
>
>> Thanks
>>
>>>
>>> Håkon
>>>
>>>> Thanks

Mark Haywood Dec. 13, 2019, 4:51 p.m. UTC | #9

On 12/12/19 9:14 AM, Mark Haywood wrote:
>
>
> On 12/12/19 7:37 AM, Håkon Bugge wrote:
>>
>>> On 12 Dec 2019, at 13:27, Leon Romanovsky <[e-mail]> wrote:
>>>
>>> On Thu, Dec 12, 2019 at 01:16:54PM +0100, Håkon Bugge wrote:
>>>>
>>>>> On 12 Dec 2019, at 13:10, Leon Romanovsky <[e-mail]> wrote:
>>>>>
>>>>> On Thu, Dec 12, 2019 at 12:59:51PM +0100, Håkon Bugge wrote:
>>>>>>
>>>>>>> On 12 Dec 2019, at 12:40, Leon Romanovsky <[e-mail]> wrote:
>>>>>>>
>>>>>>> On Wed, Dec 11, 2019 at 08:31:18PM +0100, Håkon Bugge wrote:
>>>>>>>>
>>>>>>>>> On 11 Dec 2019, at 14:13, Håkon Bugge 
>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On 11 Dec 2019, at 13:39, Leon Romanovsky <[e-mail]> 
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> On Wed, Dec 11, 2019 at 11:34:00AM +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 more accurate description is that rdma_nl_rcv_skb() 
>>>>>>>>>> always generates
>>>>>>>>>> NLMSG_ERROR without relation to NLM_F_ACK flag. The NLM_F_ACK 
>>>>>>>>>> flag is
>>>>>>>>>> requested to get acknowledges for the success.
>>>>>>>>
>>>>>>>> Yes. And when, lets say a legitimate path record response, 
>>>>>>>> containing N positive bytes, is sent back from ibacm to the 
>>>>>>>> kernel, rdma_nl_rcv_skb() think this is an error, due to "if 
>>>>>>>> (nlh->nlmsg_flags & NLM_F_ACK || err)" _and_ 
>>>>>>>> ib_nl_handle_resolve_resp() returning N.
>>>>>>> How did you test this patch?
>>>>>>> Do we have open-source applications which don't set NLM_F_ACK for
>>>>>>> ib_nl_*() calls?
>>>>>> As I alluded to above, yes, ibacm doesn't set it.
>>>>> In this regards, I'm amazed that this patch didn't break ibacm.
>>>> On the contrary. The patch avoids the kernel sending back an 
>>>> error/ACK for every path record / resolve response.
>>> As long as ibacm continues to work with this patch, i'm ok.
>>> What type of testing did you perform?
>> I'll let Mark respond to the testing. The background is that ibacm 
>> was very *liberal* when it comes checking the requests it received 
>> from the kernel. In an attempt to tighten that, Mark discovered that 
>> ibacm received an unexpected ACK from the kernel just after having 
>> sent a response.
>
>
> Without this patch, for every response to a RDMA_NL_LS_OP_RESOLVE 
> request, ibacm receives an ACK with a nlmsgerr error value equal to 
> the length of the response message.


To be clear, ibacm does nothing with the ACK other than write a log 
message that it received an unexpected request. That is why this patch 
has no ill-effect on ibacm and is why ...


>
> With this patch, no ACKs are received.


this is preferable.


>
> If I add the NLM_F_ACK to the nlmsg_header flags when responding to 
> the RDMA_NL_LS_OP_RESOLVE request, ibacm once again receives the ACKs.
>
> Mark
>
>
>>
>> That aside, I think the RDMA NL callbacks shall adhere to the 
>> RTNETLINK conventions, thus, that's why this commit changes the 
>> callbacks and not the  rdma_nl_rcv_skb().
>>
>>
>> Thxs, Håkon
>>
>>> Thanks
>>>
>>>>
>>>> Håkon
>>>>
>>>>> Thanks
>

Patch

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)