Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from misinterpreted flag

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

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

On Fri, Oct 20, 2017 at 07:04:33PM +0000, Wan, Kaike wrote:
>
>
> > -----Original Message-----
> > From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> > owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Leon Romanovsky
> > Sent: Friday, October 20, 2017 12:20 PM
> > To: Wan, Kaike
> > Cc: Ruhl, Michael J; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH] RDMA/netlink: OOPs in rdma_nl_rcv_msg() from
> > misinterpreted flag
> >
> > On Fri, Oct 20, 2017 at 12:18:02PM +0000, Wan, Kaike wrote:
> > > The nlmsg_flags from 0x100 and up have always been overloaded for
> > different requests, as shown in include/uapi/linux/netlink.h:
> >
> > Exactly, they are overloaded in include/uapi/linux/netlink.h where all nlmsg
> > flags are declared and not in some random rdma*.h file.
>
> Since they are known to be overloaded, how could you depend on the flags to make decision for all requests/responses?

Sorry for applying common sense for the RDMA subsystem. The netlink structures are declared
and exported to users in one specific file, the structure, flags and defines belong to netdev
subsystem and were not supposed to be overwritten by other users.

>
> The reason for defining RDMA_NL_LS_F_ERR in include/uapi/rdma/rdma_netlink is that all LS related defines and structures are defined in the file. It is true that it can lead to misuse, just like the current code. However, the defines in include/uapi/linux/netlink.h un-mistakenly indicate that those bits can be overloaded.

We are talking about theoretical case, the current LS implementation
will stay as is, so no worries.

>
> >
> > But it is not my main point.
> >
> > My main point is lack of usage of NLMSG_ERROR messages to inform about
> > errors, exactly as kernel informs users about errors in netlink, but in the
> > opposite direction.
>
> The difference is that the LS needs all the original information (request type, client index, msg_seq, etc) to match the response with the original request. Therefore, the easiest thing to do is to return the response with the original info plus an error flag, instead of using the NLMSG_ERROR message.
>

Just wanted to mention that NLMSG_ERROR is netlink message type and it
carries all other information except message type.

> > > >
> > > > In meanwhile, can we implement dummy dumpit functions for the LS,
> > > > which reuse ib_nl_is_good_ip_resp?
> > >
> > > Why do you want to jump all the dump hoops instead of directly calling the
> > response handler? LS is different from other netlink channels in that it sends
> > request from kernel to user space and receives responses from it instead of
> > the other way around. Consequently, the handling of netlink responses may
> > be different from handing requests from user space.
> > >
> >
> > Doesn't the part of email below answers on the question "why"?
>
> No. A response should not necessarily be processed the same as a request.

Right, and I expected to see complete mirror of kernel-to-user
communication in respect to LS user-to-kernel communication.

>
> >
> > > >
> > > > I prefer this solution over yours, because it doesn't mix
> > > > LS-specifics with general decision function and leaves LS anomalies in the
> > LS-relevant code.
> >
> >
> > > >
> > > > And returning 0 in absence of dumpit function as a response with
> > > > NLM_F_DUMP flag is wrong. User should be aware of the fact that
> > > > something wrong was with his request.
> > > >
> > > > Thanks
> > > >
> > >

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