| 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
| ||
|
Message-Id: <20100106.203423.58447620.davem@davemloft.net>
Date: Wed, 06 Jan 2010 20:34:23 -0800 (PST)
From: David Miller <davem@...emloft.net>
To: john.r.fastabend@...el.com
Cc: netdev@...r.kernel.org, herbert@...dor.apana.org.au
Subject: Re: [RFC PATCH] net: consolidate netif_needs_gso() checks
From: John Fastabend <john.r.fastabend@...el.com>
Date: Mon, 04 Jan 2010 10:21:53 +0000
> netif_needs_gso() is checked twice in the TX path once,
> before submitting the skb to the qdisc and once after
> it is dequeued from the qdisc just before calling
> ndo_hard_start(). This opens a window for a user to
> change the gso/tso or tx checksum settings that can
> cause netif_needs_gso to be true in one check and false
> in the other.
>
> Specifically, changing TX checksum setting may cause
> the warning in skb_gso_segment() to be triggered if
> the checksum is calculated earlier.
>
> This consolidates the netif_needs_gso() calls so that
> the stack only checks if gso is needed after the skb
> is dequeued from the qdisc. Or if the device has no
> queue then before dev_hard_start_xmit() in dev_queue_xmit().
>
> Signed-off-by: John Fastabend <john.r.fastabend@...el.com>
Herbert, please review.
> ---
>
> net/core/dev.c | 88 +++++++++++++++++++++++++++++++++++++-------------------
> 1 files changed, 58 insertions(+), 30 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c36a17a..91077df 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1808,12 +1808,39 @@ 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;
> + /* 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)
> @@ -2006,6 +2033,35 @@ int dev_queue_xmit(struct sk_buff *skb)
> struct Qdisc *q;
> int rc = -ENOMEM;
>
> + /* Disable soft irqs for various locks below. Also
> + * stops preemption for RCU.
> + */
> + rcu_read_lock_bh();
> +
> + txq = dev_pick_tx(dev, skb);
> + q = rcu_dereference(txq->qdisc);
> +
> +#ifdef CONFIG_NET_CLS_ACT
> + skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_EGRESS);
> +#endif
> + if (q->enqueue) {
> + rc = __dev_xmit_skb(skb, q, dev, txq);
> + goto out;
> + }
> +
> + /* The device has no queue. Common case for software devices:
> + loopback, all the sorts of tunnels...
> +
> + Really, it is unlikely that netif_tx_lock protection is necessary
> + here. (f.e. loopback and IP tunnels are clean ignoring statistics
> + counters.)
> + However, it is possible, that they rely on protection
> + made by us here.
> +
> + Check this and shot the lock. It is not prone from deadlocks.
> + Either shot noqueue qdisc, it is even simpler 8)
> + */
> +
> /* GSO will handle the following emulations directly. */
> if (netif_needs_gso(dev, skb))
> goto gso;
> @@ -2035,34 +2091,6 @@ int dev_queue_xmit(struct sk_buff *skb)
> }
>
> gso:
> - /* Disable soft irqs for various locks below. Also
> - * stops preemption for RCU.
> - */
> - rcu_read_lock_bh();
> -
> - txq = dev_pick_tx(dev, skb);
> - q = rcu_dereference(txq->qdisc);
> -
> -#ifdef CONFIG_NET_CLS_ACT
> - skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_EGRESS);
> -#endif
> - if (q->enqueue) {
> - rc = __dev_xmit_skb(skb, q, dev, txq);
> - goto out;
> - }
> -
> - /* The device has no queue. Common case for software devices:
> - loopback, all the sorts of tunnels...
> -
> - Really, it is unlikely that netif_tx_lock protection is necessary
> - here. (f.e. loopback and IP tunnels are clean ignoring statistics
> - counters.)
> - However, it is possible, that they rely on protection
> - made by us here.
> -
> - Check this and shot the lock. It is not prone from deadlocks.
> - Either shot noqueue qdisc, it is even simpler 8)
> - */
> if (dev->flags & IFF_UP) {
> int cpu = smp_processor_id(); /* ok because BHs are off */
>
> @@ -4869,7 +4897,7 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
> printk(KERN_NOTICE "%s: Dropping NETIF_F_SG since no "
> "checksum feature.\n", name);
> features &= ~NETIF_F_SG;
> - }
> +}
>
> /* TSO requires that SG is present as well. */
> if ((features & NETIF_F_TSO) && !(features & NETIF_F_SG)) {
>
> --
> 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
--
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