net: guard against coalescing packets from buggy network drivers

This patch adds a guard against coalescing packets received by buggy network drivers that don't initialize skb->tail properly.

Coalescing such packets will corrupt data in a way that makes it hard to track down the real cause of the problem. Observed symptoms are a flood of WARNs by tcp_recvmsg at random times, leaving no clear indication as to why the packet buffers were corrupted. See for example this thread:
https://groups.google.com/forum/#!topic/linux.kernel/A8ZI9aXooSU

Obviously the correct thing to do is fixing the drivers in question (eg. by using skb_put() instead of just setting skb->len) - this patch will hopefully help to track them down with less effort than I had to put into it :)

NB: in my case it was an out-of-tree Bluegiga WiFi driver; a quick look indicates that there are in-tree drivers (for hardware that I don't own) with the same bug.

index 1b62343..188a60a 100644
@@ -3833,6 +3833,19 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
         return false;
 
     if (len <= skb_tailroom(to)) {
+        /* Guard against buggy network drivers that don't initialize
+         * skb properly: if they manipulate skb->len directly without
+         * setting skb->tail, the skb_put below may return a pointer to
+         * live data (such as TCP headers) that (without this guard)
+         * would get overwritten by coalescing.
+         * The correct thing to do is fixing the network drivers - but
+         * this guard should give us a chance to spot them before they
+         * cause real hard-to-debug problems :)
+         */
+        if (WARN_ONCE(skb_tail_pointer(to) < to->data,
+                  "skb inconsistency: tail below data\n"))
+            return false;
+
         BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
         *delta_truesize = 0;
         return true;

If you want a debugging patch, this should happen way before TCP stack or even IP stack.

Say you want to use this buggy driver on a router, this code path will never bit hit.

You are right, of course, there are more effective ways to catch buggy drivers.
But they will probably also be much more expensive.
This one is very cheap, being in a relatively cold path, especially compared to the memcpy in the same path.

Svenning

It seems you missed the recent tipc thread then.

They are many ways this code path can be abused, your patch only takes care of one case.

In tipc case, they added skb to shinfo->frag_list without changing skb->data_len.

So skb_tailroom(to) was not returning 0 as it should.

The invariant should be more strict.

BUG_ON(to->data + to->len != skb_tail_pointer(to));

I still think a BUG() to catch this is better.

There is no guarantee the developer/user will catch a WARN(), the bug could sit there a long time.

I have no pity for serious bugs like that.

Since you spot buggy drivers, please provide their fixes or at least name them so that we can take care of them.

Thanks

Yes, I'm not subscribed to the mailing list at the moment - too much traffic and too little time..

Neither do I.
Either BUG or WARN would have worked for me and would have saved me a lot of time in chasing this bug in a monster size (almost 800 files!) third party driver for this unfamiliar chip on newly developed embedded hardware (so I couldn't be sure it wasn't a hardware bug) with nothing else than a serial port and a few MB flash to store log files on.

Strange, I was almost certain that I saw at least one with the same bug, but I can't seem to find it any longer. Maybe I looked too quick, or maybe it's been fixed by the git pull I've made in the meantime. I'll take a closer look if I get some spare time some day..