RE: [Xen-devel] xennet_start_xmit assumptions

-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
Sent: 19 January 2017 11:14
To: Paul Durrant <redacted>
Cc: Konrad Rzeszutek Wilk <redacted>; Wei Liu
[off-list ref]; netdev@vger.kernel.org; xen-
devel@lists.xenproject.org
Subject: Re: [Xen-devel] xennet_start_xmit assumptions

On (01/19/17 09:36), Paul Durrant wrote:

quoted

Hi Sowmini,

  Sounds like a straightforward bug to me... netfront should be able
to handle an empty skb and clearly, if it's relying on skb_headlen()
being non-zero, that's not the case.

  Paul

I see. Seems like there are 2 things broken here: recovering
from skb->len = 0, and recovering from  the more complex
case of (skb->len > 0 && skb_headlen(skb) == 0)

Do you folks want to take a shot at fixing this,
since you know the code better? If you are interested,
I can share my test program to help you reproduce the
simpler skb->len == 0 case, but it's the fully non-linear
skbs that may be more interesting to reproduce/fix.

I'll probably work on fixing packet_snd to return -EINVAL
or similar when the len is zero this week.

Sowmini,

  I knocked together the following patch, which seems to work for me:

---8<---

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 40f26b6..a957c89 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c

@@ -567,6 +567,10 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
        u16 queue_index;
        struct sk_buff *nskb;

+       /* Drop packets that are not at least ETH_HLEN in length */
+       if (skb->len < ETH_HLEN)
+               goto drop;
+
        /* Drop the packet if no queues are set up */
        if (num_queues < 1)
                goto drop;

@@ -609,6 +613,8 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
        }

        len = skb_headlen(skb);
+       if ((len < ETH_HLEN) && !__pskb_pull_tail(skb, ETH_HLEN))
+               goto drop;

        spin_lock_irqsave(&queue->tx_lock, flags);

---8<---

  Making netfront cope with a fully non-linear skb looks like it would be quite intrusive and probably not worth it so I opted for just doing the ETH_HLEN pull-tail if necessary. Can you check it works for you?

  Paul