netdev - Re: [PATCH] netpoll: Check for skb->queue

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b98cb4f6-2e4c-71ba-1431-1824bf334f8e@oracle.com>
Date:   Wed, 12 Apr 2017 15:37:27 -0700
From:   tndave <tushar.n.dave@...cle.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     davem@...emloft.net, edumazet@...gle.com, brouer@...hat.com,
        netdev@...r.kernel.org, sowmini.varadhan@...cle.com
Subject: Re: [PATCH] netpoll: Check for skb->queue_mapping



On 04/06/2017 12:14 PM, Eric Dumazet wrote:
> On Thu, 2017-04-06 at 12:07 -0700, tndave wrote:
>
>>> +			q_index = q_index % dev->real_num_tx_queues;
>> cpu interrupted here and dev->real_num_tx_queues has reduced!
>>> +			skb_set_queue_mapping(skb, q_index);
>>> +		}
>>> +		txq = netdev_get_tx_queue(dev, q_index);
>> or cpu interrupted here and dev->real_num_tx_queues has reduced!
>
> If dev->real_num_tx_queues can be changed while this code is running we
> are in deep deep trouble.
>
> Better make sure that when control path does this change, device (and/pr
> netpoll) is frozen and no packet can be sent.
When control path is making change to real_num_tx_queues, underlying
device is disabled; also netdev tx queues are stopped/disabled so
certainly no transmit is happening.


The corner case I was referring is if netpoll's queue_process() code is
interrupted and while it is not running, control path makes change to
dev->real_num_tx_queues and exits. Later on, interrupted queue_process()
resume execution and it can end up with wrong skb->queue_mapping and txq.
We can prevent this case with below change:

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 9424673..29be246 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -105,15 +105,21 @@ static void queue_process(struct work_struct *work)
         while ((skb = skb_dequeue(&npinfo->txq))) {
                 struct net_device *dev = skb->dev;
                 struct netdev_queue *txq;
+               unsigned int q_index;

                 if (!netif_device_present(dev) || !netif_running(dev)) {
                         kfree_skb(skb);
                         continue;
                 }

-               txq = skb_get_tx_queue(dev, skb);
-
                 local_irq_save(flags);
+               /* check if skb->queue_mapping is still valid */
+               q_index = skb_get_queue_mapping(skb);
+               if (unlikely(q_index >= dev->real_num_tx_queues)) {
+                       q_index = q_index % dev->real_num_tx_queues;
+                       skb_set_queue_mapping(skb, q_index);
+               }
+               txq = netdev_get_tx_queue(dev, q_index);
                 HARD_TX_LOCK(dev, txq, smp_processor_id());
                 if (netif_xmit_frozen_or_stopped(txq) ||
                     netpoll_start_xmit(skb, dev, txq) != NETDEV_TX_OK) {


Thanks for your patience.

-Tushar
>
>
>
>