netdev - Re: NULL pointer dereference panic in stable (2.6.33.2), amd64

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]

Date:	Mon, 12 Apr 2010 09:08:34 +0530
From:	Krishna Kumar2 <krkumar2@...ibm.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	Denys Fedorysychenko <nuclearcat@...learcat.com>
Subject: Re: NULL pointer dereference panic in stable (2.6.33.2), amd64

Hi Eric,

Eric Dumazet <eric.dumazet@...il.com> wrote on 04/12/2010 04:05:53 AM:

> I believe the following lines from dev_pick_tx() are not the problem :
>
>    if (sk && sk->sk_dst_cache)
>       sk_tx_queue_set(sk, queue_index);
>
> It is IMHO not safe, because route for this socket might have just
> changed and we are transmitting an old packet (queued some milli seconds
> before, when route was different).
>
> We then memorize a queue_index that might be too big for the new device
> of new selected route.
>
> Next packet we want to transmit will take the cached value of
> queue_index, correct for old device, maybe not correct for new device.

When route changes, I think my patch had reset sk->sk_tx_queue_mapping
by calling sk_tx_queue_clear. I don't know if I missed any path where
the route changes and sk_dst_reset() was not called.

The following might be better to prove the panic is due to this, since
your suggestion will hide a panic that happens somewhat rare (according
to Denys):

      if (sk_tx_queue_recorded(sk)) {
            queue_index = sk_tx_queue_get(sk);
+           queue_index = dev_cap_txqueue(dev, queue_index);
      } else {

Thanks,

- KK

> You could try to revert commit a4ee3ce3293dc931fab19beb472a8bde1295aebe
>
> commit a4ee3ce3293dc931fab19beb472a8bde1295aebe
> Author: Krishna Kumar <krkumar2@...ibm.com>
> Date:   Mon Oct 19 23:50:07 2009 +0000
>
>     net: Use sk_tx_queue_mapping for connected sockets
>
>     For connected sockets, the first run of dev_pick_tx saves the
>     calculated txq in sk_tx_queue_mapping. This is not saved if
>     either the device has a queue select or the socket is not
>     connected. Next iterations of dev_pick_tx uses the cached value
>     of sk_tx_queue_mapping.
>
>     Signed-off-by: Krishna Kumar <krkumar2@...ibm.com>
>     Signed-off-by: David S. Miller <davem@...emloft.net>
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 28b0b9e..fa88dcd 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1791,13 +1791,25 @@ EXPORT_SYMBOL(skb_tx_hash);
>  static struct netdev_queue *dev_pick_tx(struct net_device *dev,
>                 struct sk_buff *skb)
>  {
> -   const struct net_device_ops *ops = dev->netdev_ops;
> -   u16 queue_index = 0;
> +   u16 queue_index;
> +   struct sock *sk = skb->sk;
> +
> +   if (sk_tx_queue_recorded(sk)) {
> +      queue_index = sk_tx_queue_get(sk);
> +   } else {
> +      const struct net_device_ops *ops = dev->netdev_ops;
>
> -   if (ops->ndo_select_queue)
> -      queue_index = ops->ndo_select_queue(dev, skb);
> -   else if (dev->real_num_tx_queues > 1)
> -      queue_index = skb_tx_hash(dev, skb);
> +      if (ops->ndo_select_queue) {
> +         queue_index = ops->ndo_select_queue(dev, skb);
> +      } else {
> +         queue_index = 0;
> +         if (dev->real_num_tx_queues > 1)
> +            queue_index = skb_tx_hash(dev, skb);
> +
> +         if (sk && sk->sk_dst_cache)
> +            sk_tx_queue_set(sk, queue_index);
> +      }
> +   }
>
>     skb_set_queue_mapping(skb, queue_index);
>     return netdev_get_tx_queue(dev, queue_index);
>
>

--
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