multiqueue, skb_get_queue_mapping() and netdev_get_tx_queue()

multiqueue, skb_get_queue_mapping() and netdev_get_tx_queue()

[Eldon Koyle]

It looks like there is a potential for an out of bounds index anywhere
skb_get_queue_mapping(skb) (which just returns skb->queue_mapping) is
used to get an index for netdev_get_tx_queue() (and probably other
places) on a device with multiple rx/tx queues.

As I understand it, skb->queue_mapping should contain rx_queue + 1,
which can be out of range for netdev_get_tx_queue (which expects a
0-based index).

Am I misunderstanding something, or should all of these occurrences be
replaced with something more like the following?

static inline u16 skb_get_queue_index(const struct sk_buff *skb)
{
        return skb->queue_mapping ? skb->queue_mapping - 1 : 0;
}

Here is how it is commonly used (which looks incorrect to me):
In net/8021q/vlan_dev.c:
	static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
						    struct net_device *dev)
	{
		int i = skb_get_queue_mapping(skb);
		struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
	...


And here is some other possibly pertinent code:
In include/linux/netdevice.h:
	static inline
	struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev,
						 unsigned int index)
	{
		return &dev->_tx[index];
	}

In net/core/dev.c:
	struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
			void (*setup)(struct net_device *), unsigned int queue_count)
	...
		tx = kcalloc(queue_count, sizeof(struct netdev_queue), GFP_KERNEL);
	...
		dev->_tx = tx;
	...

In include/linux/skbuff.h:
	static inline u16 skb_get_queue_mapping(const struct sk_buff *skb)
	{
		return skb->queue_mapping;
	}
	...
	static inline void skb_record_rx_queue(struct sk_buff *skb, u16 rx_queue)
	{
		skb->queue_mapping = rx_queue + 1;
	}

	static inline u16 skb_get_rx_queue(const struct sk_buff *skb)
	{
		return skb->queue_mapping - 1;
	}


-- 
Eldon Koyle
-- 
Politicians are the same all over.  They promise to build a bridge even
where there is no river.
		-- Nikita Khrushchev

Re: multiqueue, skb_get_queue_mapping() and netdev_get_tx_queue()

[Eldon Koyle]

On  Jul 14 17:13-0600, Eldon Koyle wrote:
> It looks like there is a potential for an out of bounds index anywhere
> skb_get_queue_mapping(skb) (which just returns skb->queue_mapping) is
> used to get an index for netdev_get_tx_queue() (and probably other
> places) on a device with multiple rx/tx queues.

Looking more closely, it looks like skb->queue_mapping is treated
differently between rx and tx.  In net/core/dev.c in dev_pick_tx, it
uses skb_tx_hash to get the tx queue it should use and then does:
	skb_set_queue_mapping(skb, queue_index);
	return netdev_get_tx_queue(dev, queue_index);

Sorry for the noise.

-- 
Eldon Koyle
-- 
He who renders warfare fatal to all engaged in it will be the greatest
benefactor the world has yet known.
		-- Sir Richard Burton