Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2010-08-13 at 16:48 +0200, Eric Dumazet wrote:

> >                  * If this skb has a frag_list, then here that means that
> >                  * we will have to use the frag_list skb for compat tasks
> >                  * and the regular skb for non-compat tasks.
> >                  *
> >                  * The skb might (and likely will) be cloned, so we can't
> >                  * just reset frag_list and go on with things -- we need to
> >                  * keep that. For the compat case that's easy -- simply get
> >                  * a reference to the compat skb and free the regular one
> >                  * including the frag. For the non-compat case, we need to
> >                  * avoid sending the frag to the user -- so assign NULL but
> >                  * restore it below before freeing the skb.
> > 
> > and that's no longer true, afaict.
> > 
> 
> Comment was not updated by the patch.
> 
> But do you agree temporarly setting frag_list to NULL was a bug ?

No, that was actually intentional, as the comment describes?

> Unfortunatly I cannot test this path...

No wireless devices? :)

> I assume reverting 1235f504aaba removes these errors ?

I haven't tried yet, but it only happened very recently and I didn't
find any other candidate -- the error always points to
wireless_send_event too.

> Its strange we have a double-free on a data part and not a skb_head.

There also was an apparent use-after-free error.

> maybe pskb_copy() has a problem with frag_list...
> 
> 
> Maybe we can revert the patch and find another way to make sure two
> process can not manipulate this skb in // (adding a mutex, or using
> RTNL ?)

I'm getting confused. Why did I think the NULLing and then restoring was
not racy to start with?

johannes