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
| ||
|
Date: Sat, 06 Mar 2010 11:27:50 -0800 From: John Fastabend <john.r.fastabend@...el.com> To: Herbert Xu <herbert@...dor.apana.org.au> CC: David Miller <davem@...emloft.net>, "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "gospo@...hat.com" <gospo@...hat.com> Subject: Re: [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks Herbert Xu wrote: > On Sat, Feb 27, 2010 at 03:27:25AM -0800, David Miller wrote: > >> If we have ip_summed == CHECKSUM_PARTIAL might some classifier >> or packet scheduler action module require that the >> transport header is setup properly before the SKB gets into >> there? >> > > I think this is OK as the transport header setting was only there > for backwards compatibility with certain drivers that relied on it. > Its use was very much isolated. > > I just did a grep on net/sched and couldn't see anything obvious > that uses transport_header. > > >>> diff --git a/net/core/dev.c b/net/core/dev.c >>> index eb7f1a4..626124d 100644 >>> --- a/net/core/dev.c >>> +++ b/net/core/dev.c >>> @@ -1835,12 +1835,40 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev, >>> { >>> const struct net_device_ops *ops = dev->netdev_ops; >>> int rc = NETDEV_TX_OK; >>> + int need_gso = netif_needs_gso(dev, skb); >>> + >>> + if (!need_gso) { >>> + if (skb_has_frags(skb) && >>> + !(dev->features & NETIF_F_FRAGLIST) && >>> + __skb_linearize(skb)) >>> + goto out_kfree_skb; >>> + >>> + /* Fragmented skb is linearized if device does not support SG, >>> + * or if at least one of fragments is in highmem and device >>> + * does not support DMA from it. >>> + */ >>> + if (skb_shinfo(skb)->nr_frags && >>> + (!(dev->features & NETIF_F_SG) || >>> + illegal_highdma(dev, skb)) && >>> + __skb_linearize(skb)) >>> + goto out_kfree_skb; >>> > > Please use skb_needs_linearize. > > >>> + /* If packet is not checksummed and device does not support >>> + * checksumming for this protocol, complete checksumming here. >>> + */ >>> + if (skb->ip_summed == CHECKSUM_PARTIAL) { >>> + skb_set_transport_header(skb, skb->csum_start - >>> + skb_headroom(skb)); >>> + if (!dev_can_checksum(dev, skb) && >>> + skb_checksum_help(skb)) >>> + goto out_kfree_skb; >>> + } >>> + } >>> >>> if (likely(!skb->next)) { >>> if (!list_empty(&ptype_all)) >>> dev_queue_xmit_nit(skb, dev); >>> >>> - if (netif_needs_gso(dev, skb)) { >>> + if (need_gso) { >>> if (unlikely(dev_gso_segment(skb))) >>> goto out_kfree_skb; >>> if (skb->next) >>> > > That whole if block should be moved into an else clause here: > > if (netif_needs_gso(dev, skb)) { > if (unlikely(dev_gso_segment(skb))) > goto out_kfree_skb; > if (skb->next) > goto gso; > } else { > do your thing > } > > The reason is that the other paths only act on the fragments > generated by GSO, so logically with your change we shouldn't > apply any further software emulation to them, even if the device > setting changed. > > Cheers, > Herbert, It looks like dev_gso_segment() could be used to "Verify header integrity only" according to the comment? If this is true I think the logic should probably be if (netif_needs_gso(dev, skb)) { if (unlikely(dev_gso_segment(skb))) goto out_kfree_skb; if (skb->next) goto gso; } do your thing That way we linearize the skb if necessary in the case were dev_gso_segment() only verifies the header and does not return a list of segments. thanks, John. -- 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
Powered by blists - more mailing lists