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: Fri, 08 Jun 2012 00:42:25 -0700 From: John Fastabend <john.r.fastabend@...el.com> To: Eric Dumazet <eric.dumazet@...il.com> CC: David Miller <davem@...emloft.net>, therbert@...gle.com, netdev@...r.kernel.org Subject: Re: [PATCH] bonding: Fix corrupted queue_mapping On 6/8/2012 12:23 AM, Eric Dumazet wrote: > On Fri, 2012-06-08 at 08:47 +0200, Eric Dumazet wrote: >> On Thu, 2012-06-07 at 23:15 -0700, David Miller wrote: >>> From: Eric Dumazet <eric.dumazet@...il.com> >>> Date: Fri, 08 Jun 2012 08:11:21 +0200 >>> >>>> On Thu, 2012-06-07 at 23:02 -0700, David Miller wrote: >>>>> Hmmm, isn't that what qdisc_skb_cb is for? And even private data is >>>>> explicitly allocated: >>>>> >>>>>> unsigned char data[24]; >>>>> >>>>> there. :-) >>>>> >>>> >>>> Yes, but some other layers can use the same trick so it might collide. >>>> >>>> Inserting the bond field in qdisc_skb_cb (level0) is safer. >>> >>> Do you suggest that Infiniband does the same thing? :-) >> >> I wonder if another way to solve this is not letting ndo_select_queue() >> method the responsibility to call skb_set_queue_mapping() itself ? >> >> (ie removing skb_set_queue_mapping() done in dev_pick_tx()) >> >> bonding would not have to save/restore skb queue mapping ? >> >> Partial patch : (we have to audit all ndo_select_queue() >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> index cd09819..c6c92d5 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -2368,6 +2368,7 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev, >> >> if (dev->real_num_tx_queues == 1) >> queue_index = 0; >> + skb_set_queue_mapping(skb, queue_index); >> else if (ops->ndo_select_queue) { >> queue_index = ops->ndo_select_queue(dev, skb); >> queue_index = dev_cap_txqueue(dev, queue_index); >> @@ -2391,9 +2392,9 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev, >> sk_tx_queue_set(sk, queue_index); >> } >> } >> + skb_set_queue_mapping(skb, queue_index); >> } >> >> - skb_set_queue_mapping(skb, queue_index); >> return netdev_get_tx_queue(dev, queue_index); >> } >> >> > > > I must say I dont understand dev_pick_tx() anymore. > > It seems to ignore skb->queue_mapping (unless device provides its own > ndo_select_queue() and this functions is aware of skb->queue_mapping, as > correctly done in ixgbe) > > So commit fff3269907897ee (tcp: reflect SYN queue_mapping into SYNACK > packets) works on ixgbe, but probably not on other multiqueue devices. > > This sounds like a regression to me. > > > Well it would get picked up via skb_tx_hash(), else if (ops->ndo_select_queue) { [...] } else { struct sock *sk = skb->sk; queue_index = sk_tx_queue_get(sk); if (queue_index < 0 || skb->ooo_okay || queue_index >= dev->real_num_tx_queues) { int old_index = queue_index; queue_index = get_xps_queue(dev, skb); if (queue_index < 0) queue_index = skb_tx_hash(dev, skb); [...] So think this might be OK. -- 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