Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Daniel Borkmann <daniel@iogearbox.net>]

On 09/02/2015 02:04 AM, Ken-ichirou MATSUZAWA wrote:
> On Wed, Aug 19, 2015 at 04:29:32PM +0200, Daniel Borkmann wrote:
>> On 08/17/2015 11:02 PM, David Miller wrote:
>>> From: Daniel Borkmann <daniel@iogearbox.net>
>>> Date: Fri, 14 Aug 2015 12:38:21 +0200
>>>
>>>> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
>>>> index 67d2104..4307446 100644
>>>> --- a/net/netlink/af_netlink.c
>>>> +++ b/net/netlink/af_netlink.c
>>>> @@ -238,6 +238,13 @@ static void __netlink_deliver_tap(struct sk_buff
>>>> *skb)
>>>>
>>>>   static void netlink_deliver_tap(struct sk_buff *skb)
>>>>   {
>>>> +	/* Netlink mmaped skbs must not access shared info, and thus
>>>> +	 * are not allowed to be cloned. For now, just don't allow
>>>> +	 * them to get inspected by taps.
>>>> +	 */
>>>> +	if (netlink_skb_is_mmaped(skb))
>>>> +		return;
>>>> +
>>>
>>> I would seriously rather see us do an expensive full copy of the SKB
>>> than to have traffic which is unexpectedly invisible to taps.
>>
>> Do you mean generically as we do in TX path, or only in this
>> particular scenario?
>
> It seems that we only handle this particular scenario.
>
> I think I can understand the cause of the panic. The original mmaped
> skb, allocated by netlink_alloc_skb, has a skb destructor which set
> skb head NULL. This prevents from accessing shared info in
> skb_release_all, so my concerns that the shared info of original skb
> may be accessed in kfree_skb and may cause a panic, is unnecessary.
>
> But since skb_clone does not copy the destructor, skb_release_all
> calls skb_release_data for cloned skb, accessing shared info and
> causes the panic, I think. Setting the destructor after clone could
> not fix the problem since __dev_queue_xmit call path,
> netif_skb_features?, accesses shared info.
>
> Talking about skb_copy path, original skb's shared info is accessed
> only in copy_skb_header, to get gso related field. As a result of
> those, we can avoid the panic by:
>
>    @@ -205,7 +205,10 @@ static int __netlink_deliver_tap_skb(struct sk_buff *skb,
>          int ret = -ENOMEM;
>
>          dev_hold(dev);
>          -       nskb = skb_clone(skb, GFP_ATOMIC);
>          +       if (netlink_skb_is_mmaped(skb))
>          +               nskb = skb_copy(skb, GFP_ATOMIC);
>          +       else
>          +               nskb = skb_clone(skb, GFP_ATOMIC);
>
> Thanks to you, my question become clear:
> Should we set gso_size to 0 after copying to make skb_is_gso
> returning false?

It's still not correct. The thing is you can neither call skb_copy() nor
skb_clone() on netlink mmaped skbs. For example, skb_copy_bits() would
look at frags[] in the shared info and tries to copy them. I think adding
extra logic to skb_copy() would be very ugly just to accommodate for this
special case. We need an own netlink_mmap_to_full_skb() handler for this,
that copies/transforms this into a "normal" skb. I'll have a look it this
week.

Thanks,
Daniel