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

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- 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 --]