[PATCH for-4.14-rc] RDMA/netlink: Restore nlmsg_len calculation in ibnl_put_attr

[PATCH for-4.14-rc] RDMA/netlink: Restore nlmsg_len calculation in ibnl_put_attr

[Shiraz Saleem]

Commit 1a1c116f3dcf removes nlmsg_len calculation in
ibnl_put_attr causing netlink messages to be rejected due
to incorrect length.

Restore the netlink message header length calculation
to include the added attribute.

Fixes: 1a1c116f3dcf ("RDMA/netlink: Simplify the put_msg and put_attr")
Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/core/netlink.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
index e685148..889d091 100644
--- a/drivers/infiniband/core/netlink.c
+++ b/drivers/infiniband/core/netlink.c
@@ -150,10 +150,14 @@ EXPORT_SYMBOL(ibnl_put_msg);
 int ibnl_put_attr(struct sk_buff *skb, struct nlmsghdr *nlh,
 		  int len, void *data, int type)
 {
+	unsigned char *prev_tail;
+
+	prev_tail = skb_tail_pointer(skb);
 	if (nla_put(skb, type, len, data)) {
 		nlmsg_cancel(skb, nlh);
 		return -EMSGSIZE;
 	}
+	nlh->nlmsg_len += skb_tail_pointer(skb) - prev_tail;
 	return 0;
 }
 EXPORT_SYMBOL(ibnl_put_attr);
-- 
2.8.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-4.14-rc] RDMA/netlink: Restore nlmsg_len calculation in ibnl_put_attr

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]

On Thu, Sep 28, 2017 at 06:49:53AM -0500, Shiraz Saleem wrote:
> Commit 1a1c116f3dcf removes nlmsg_len calculation in
> ibnl_put_attr causing netlink messages to be rejected due
> to incorrect length.
>
> Restore the netlink message header length calculation
> to include the added attribute.
>
> Fixes: 1a1c116f3dcf ("RDMA/netlink: Simplify the put_msg and put_attr")
> Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/infiniband/core/netlink.c | 4 ++++
>  1 file changed, 4 insertions(+)
>

The length supposed to be updated in ibnl_put_msg, where you should
supply correct length from the beginning. The suggested way to supply
length for unknown data is to provide NLMSG_DEFAULT_SIZE while allocating
new netlink message.

NLMSG_DEFAULT_SIZE ensures that netlink fits into one page.

It is better to avoid messing with message length after allocations,
especially for the attributes.

Thanks

> diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
> index e685148..889d091 100644
> --- a/drivers/infiniband/core/netlink.c
> +++ b/drivers/infiniband/core/netlink.c
> @@ -150,10 +150,14 @@ EXPORT_SYMBOL(ibnl_put_msg);
>  int ibnl_put_attr(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		  int len, void *data, int type)
>  {
> +	unsigned char *prev_tail;
> +
> +	prev_tail = skb_tail_pointer(skb);
>  	if (nla_put(skb, type, len, data)) {
>  		nlmsg_cancel(skb, nlh);
>  		return -EMSGSIZE;
>  	}
> +	nlh->nlmsg_len += skb_tail_pointer(skb) - prev_tail;
>  	return 0;
>  }
>  EXPORT_SYMBOL(ibnl_put_attr);
> --
> 2.8.3
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-4.14-rc] RDMA/netlink: Restore nlmsg_len calculation in ibnl_put_attr

[Shiraz Saleem]

On Thu, Sep 28, 2017 at 04:24:57PM +0300, Leon Romanovsky wrote:
> On Thu, Sep 28, 2017 at 06:49:53AM -0500, Shiraz Saleem wrote:
> > Commit 1a1c116f3dcf removes nlmsg_len calculation in
> > ibnl_put_attr causing netlink messages to be rejected due
> > to incorrect length.
> >
> > Restore the netlink message header length calculation
> > to include the added attribute.
> >
> > Fixes: 1a1c116f3dcf ("RDMA/netlink: Simplify the put_msg and put_attr")
> > Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/infiniband/core/netlink.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> 
> The length supposed to be updated in ibnl_put_msg, where you should
> supply correct length from the beginning. The suggested way to supply
> length for unknown data is to provide NLMSG_DEFAULT_SIZE while allocating
> new netlink message.
> 
> NLMSG_DEFAULT_SIZE ensures that netlink fits into one page.
> 
> It is better to avoid messing with message length after allocations,
> especially for the attributes.
> 

Hi Leon -

It is not neccessary that the length is passed in ibnl_put_msg.

See drivers/infiniband/core/addr.c/ib_nl_ip_send_msg().  

In ib_nl_ip_send_msg() nlmsg_len is corrected using nlmsg_end,
which is typically called after attributes are added to calculate
the correct length.

The _original_ ibnl_put_attr wrapper in netlink.c achieves the same
by calculating the nlmsg_len.

One can argue that it is undesirable to calculate nlmsg_len on 
every ibnl_put_attr call. An alternate fix we could do is instead of
recalculating the length with every ibnl_put_attr call is to
add a nlmsg_end after the last ibnl_put_attr call.

Do you agree this is a reasonable solution to fix your commit?

Shiraz

 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-4.14-rc] RDMA/netlink: Restore nlmsg_len calculation in ibnl_put_attr

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]

On Thu, Sep 28, 2017 at 02:00:33PM -0500, Shiraz Saleem wrote:
> On Thu, Sep 28, 2017 at 04:24:57PM +0300, Leon Romanovsky wrote:
> > On Thu, Sep 28, 2017 at 06:49:53AM -0500, Shiraz Saleem wrote:
> > > Commit 1a1c116f3dcf removes nlmsg_len calculation in
> > > ibnl_put_attr causing netlink messages to be rejected due
> > > to incorrect length.
> > >
> > > Restore the netlink message header length calculation
> > > to include the added attribute.
> > >
> > > Fixes: 1a1c116f3dcf ("RDMA/netlink: Simplify the put_msg and put_attr")
> > > Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  drivers/infiniband/core/netlink.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> >
> > The length supposed to be updated in ibnl_put_msg, where you should
> > supply correct length from the beginning. The suggested way to supply
> > length for unknown data is to provide NLMSG_DEFAULT_SIZE while allocating
> > new netlink message.
> >
> > NLMSG_DEFAULT_SIZE ensures that netlink fits into one page.
> >
> > It is better to avoid messing with message length after allocations,
> > especially for the attributes.
> >
>
> Hi Leon -
>
> It is not neccessary that the length is passed in ibnl_put_msg.
>
> See drivers/infiniband/core/addr.c/ib_nl_ip_send_msg().
>
> In ib_nl_ip_send_msg() nlmsg_len is corrected using nlmsg_end,
> which is typically called after attributes are added to calculate
> the correct length.
>
> The _original_ ibnl_put_attr wrapper in netlink.c achieves the same
> by calculating the nlmsg_len.
>
> One can argue that it is undesirable to calculate nlmsg_len on
> every ibnl_put_attr call. An alternate fix we could do is instead of
> recalculating the length with every ibnl_put_attr call is to
> add a nlmsg_end after the last ibnl_put_attr call.
>
> Do you agree this is a reasonable solution to fix your commit?

As long as you don't add it into ibnl_put_attr and add nlmsg_end into the caller sites,
I'm more than fine with that.

The rationale behind my "simplify ..." commit was to get rid of ibnl_put_attr
and replace all calls to appropriate nla_* calls which performs type
checking.

Thanks

>
> Shiraz
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]