On Wed, Aug 19, 2015 at 04:29:32PM +0200, Daniel Borkmann wrote:

On 08/17/2015 11:02 PM, David Miller wrote:

quoted

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Fri, 14 Aug 2015 12:38:21 +0200

quoted

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?

Thanks,