netdev - Re: [PATCH v4 06/10] e1000e: Support for byte queue limits

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+mtBx80aoWtDh3=gMnfVCi9r5E_bjqgreoa-yjwDc5+kcocjg@mail.gmail.com>
Date:	Thu, 1 Dec 2011 08:32:16 -0800
From:	Tom Herbert <therbert@...gle.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v4 06/10] e1000e: Support for byte queue limits

> OK, as stated on your other thread, its obvious this driver (and
> probably other intel drivers) made assumptions that are now obsolete,
> since skb head can contain some data payload, not only (MAC+IP+TCP)
> headers.
>
Looks like similar is in several Intel drivers.  Only other driver I
found that is using the false assumption is tg3 IPv6 gso path.

> If Intel guys cannot afford approximate the bytecount by skb->len, I
> suggest to use same trick found in
> drivers/net/ethernet/intel/igb/igb_main.c
>
> static int igb_tso(struct igb_ring *tx_ring,
> ...
>        /* compute header lengths */
>        l4len = tcp_hdrlen(skb);
>        *hdr_len = skb_transport_offset(skb) + l4len;
>
I noticed a similar technique used by several drivers.  it's fine if
it already knew the packet was TCP.  Just assuming that a GSO packet
is TCP might be a new bug, so in the e1000e driver they would need to
check the type (gso_type == SKB_GSO_TCPV4 or SKB_GSO_TCPV6).

I'm not sure that tcp_hdrlen is even being used correctly in the other
drivers.  For instance in bnx2x, tcp_hdrlen is called after checking
skb_is_gso_v6(skb) and skb_is_gso(skb).  If the former is true then in
fact it is a TCP packet (gso_type is SKB_GSO_TCPV6) , but the if the
latter is true it only proves that is is GSO packet (gso_type not
checked, only gso_size>0)--  the packet might be a fragment UDP packet
for instance, so tcphdr_len is not valid on those skbs.
--
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