| Message ID | 20170928114953.9600-1-shiraz.saleem@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@intel.com> > Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com> > --- > 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 >
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@intel.com> > > Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com> > > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@intel.com> > > > Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com> > > > --- > > > 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 > > >
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);
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@intel.com> Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com> --- drivers/infiniband/core/netlink.c | 4 ++++ 1 file changed, 4 insertions(+)