netdev - Re: removing the timer from cdc-ncm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87k3uxgq6g.fsf@nemi.mork.no>
Date:	Thu, 11 Oct 2012 14:15:51 +0200
From:	Bjørn Mork <bjorn@...k.no>
To:	Oliver Neukum <oneukum@...e.de>
Cc:	Alexey ORISHKO <alexey.orishko@...ricsson.com>,
	"netdev\@vger.kernel.org" <netdev@...r.kernel.org>,
	"linux-usb\@vger.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: removing the timer from cdc-ncm

Bjørn Mork <bjorn@...k.no> writes:

> [48880.048494] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
> [48880.048573] IP: [<ffffffffa06ba879>] cdc_ncm_tx_bundle+0x168/0x43b [cdc_ncm]

This one is because you removed the "if (skb == NULL)" from the for
loop, but left the "skb = NULL;" at the end:


@@ -719,28 +707,15 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb)
                /* compute maximum buffer size */
                rem = ctx->tx_max - offset;
 
-               if (skb == NULL) {
-                       skb = ctx->tx_rem_skb;
-                       ctx->tx_rem_skb = NULL;
-
-                       /* check for end of skb */
-                       if (skb == NULL)
-                               break;
-               }
-
                if (skb->len > rem) {
                        if (n == 0) {
                                /* won't fit, MTU problem? */
                                dev_kfree_skb_any(skb);
                                skb = NULL;
                                ctx->netdev->stats.tx_dropped++;
+                               error = 1;
                        } else {
-                               /* no room for skb - store for later */
-                               if (ctx->tx_rem_skb != NULL) {
-                                       dev_kfree_skb_any(ctx->tx_rem_skb);
-                                       ctx->netdev->stats.tx_dropped++;
-                               }
-                               ctx->tx_rem_skb = skb;
+
                                skb = NULL;
                                ready2send = 1;
                        }
@@ -768,13 +743,6 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb)
                skb = NULL;
        }


If I understand your intentions with this code, then the for loop should
probably just go away completely?

Anyway, after avoiding that one, I ended up with

[  857.112692] cdc_ncm_fill_tx_frame: skb_out=ffff880218624cc0, length=2048
[  857.112696] cdc_ncm_tx_fixup: returning           (null)
[  857.112731] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
[  857.112807] IP: [<ffffffffa030e68b>] usbnet_start_xmit+0x128/0x4e9 [usbnet]

No need for the reset of the trace here. Removing the "goto not_drop"
makes usbnet_start_xmit continue after tx_fixup returns NULL:


-       if (info->tx_fixup) {
+       if (transmit_now && info->tx_fixup) {
                skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
                if (!skb) {
                        if (netif_msg_tx_err(dev)) {
                                netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
                                goto drop;
-                       } else {
-                               /* cdc_ncm collected packet; waits for more */
-                               goto not_drop;
                        }
                }
        }


	// some devices want funky USB-level framing, for
	// win32 driver (usually) and/or hardware quirks
	if (transmit_now && info->tx_fixup) {
		skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
		if (!skb) {
			if (netif_msg_tx_err(dev)) {
				netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
				goto drop;
			}
		}
	}
	length = skb->len;



I stopped there.  I am not exactly sure where you were going with this
anymore.  tx_fixup will return the tx_curr_skb being currently built
every time it is called, without anything resetting it at that point.
Then when finally cdc_ncm_fill_tx_frame has been called enough times to
fill it completely, you end up just dropping it on the floor.  The
result is that nothing is ever transmitted even after fixing the above
errors because the tx_curr_skb has an incomplete header every time
tx_fixup returns it.

I do like your idea, but if you don't mind I think we'll go ahead and
base the upcoming cdc_mbim driver on the current cdc_ncm *with* the
timer.  The plan is to reuse as much of cdc_ncm as possible, so in
theory it should be easy to update both drivers when the timer goes away
(there is no need for cdc_mbim be aware of the timer at all).  But
unfortunately it seems that cdc_ncm_fill_tx_frame must be changed a lot
to support the concept of multiple NDPs in a single NTB.  This means
that the timer removal patches will conflict heavily with the
preparations for MBIM support.  With some luck and acceptance from all
contributors, I hope to be able to post the first version of that as the
merge window closes.



Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html