Re: [PATCH] TIPC: Fix infinite loop in netlink handler

[Florian Westphal <fw-kernel@strlen.de>]

Stephens, Allan <allan.stephens@windriver.com> wrote:
[removed tipc-discussion list from CC]
> Patrick McHardy wrote:
> > Florian Westphal wrote:
> > > -		genlmsg_unicast(rep_buf, req_nlh->nlmsg_pid);
> > > +		genlmsg_unicast(rep_buf, NETLINK_CB(skb).pid);
> > 
> > 
> > This is the second time we're seeing this bug within a few 
> > weeks, maybe we should rename NETLINK_CB(skb).pid to dst_pid 
> > to avoid similar confusion in the future? We could even 
> > rename nlmsg_pid to nlmsg_src within the kernel, which should 
> > make it completely clear what is being refered to.
> 
> I'm assuming that Patrick's first suggestion should have read "rename
> NETLINK_CB(skb).pid to src_pid", as there is already a "dst_pid" field
> in the netlink_skb_parms structure type.

Hmm, this dst_pid got removed in commit
4e9b82693542003b028c8494e9e3c49615b91ce7.

And im actually not sure if a rename would really help things.
The problem was/is the usage of the original header value (which works just
fine if userspace plays along). So the only real solution
would be to update the nlmsg_pid value to the NETLINK_CB(skb).pid if
it is 0 before the 'doit' callback in genl_rcv_msg() -- and this seems
just the wrong thing to to. Plus, there is a snd_seq field in the info
structure passed to the callback which always has the NETLINK_CB(skb).pid.


Perhaps something like the following would be a start?

./include/linux/netlink.h:
-         __u32           nlmsg_pid;      /* Sending process port ID */
+         __u32           nlmsg_pid;      /* Sending process port ID -
+  must not be used as destination parameter with
+  genlmsg_unicast() and friends as it might be 0 meaning 'send to kernel' */

Thoughts?