| Message ID | 20230317181941.86151-2-nnac123@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/2] net: Catch invalid index in XPS mapping | expand |
On Fri, Mar 17, 2023 at 01:19:41PM -0500, Nick Child wrote: > When requesting a TX queue at a given index, prevent out-of-bounds > referencing by ensuring that the index is within the allocated number > of queues. > > If there is an out-of-bounds reference then inform the user and return > a reference to the first tx queue instead. > > Fixes: e8a0464cc950 ("netdev: Allocate multiple queues for TX.") > Signed-off-by: Nick Child <nnac123@linux.ibm.com> > --- > include/linux/netdevice.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 23b0d7eaaadd..fe88b1a7393d 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -2482,6 +2482,13 @@ static inline > struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev, > unsigned int index) > { > + if (unlikely(index >= dev->num_tx_queues)) { > + net_warn_ratelimited("%s selects TX queue %d, but number of TX queues is %d\n", > + dev->name, index, > + dev->num_tx_queues); > + return &dev->_tx[0]; Why return first queue here instead of NULL, wouldn't that confuse the caller instead of return proper (NULL) value? Piotr. > + } > + > return &dev->_tx[index]; > } > > -- > 2.31.1 >
On 3/17/23 13:45, Piotr Raczynski wrote: > On Fri, Mar 17, 2023 at 01:19:41PM -0500, Nick Child wrote: >> When requesting a TX queue at a given index, prevent out-of-bounds >> referencing by ensuring that the index is within the allocated number >> of queues. >> >> If there is an out-of-bounds reference then inform the user and return >> a reference to the first tx queue instead. >> >> Fixes: e8a0464cc950 ("netdev: Allocate multiple queues for TX.") >> Signed-off-by: Nick Child <nnac123@linux.ibm.com> >> --- >> include/linux/netdevice.h | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index 23b0d7eaaadd..fe88b1a7393d 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -2482,6 +2482,13 @@ static inline >> struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev, >> unsigned int index) >> { >> + if (unlikely(index >= dev->num_tx_queues)) { >> + net_warn_ratelimited("%s selects TX queue %d, but number of TX queues is %d\n", >> + dev->name, index, >> + dev->num_tx_queues); >> + return &dev->_tx[0]; > > Why return first queue here instead of NULL, wouldn't that confuse the > caller instead of return proper (NULL) value? > Thanks for reviewing Piotr. netdev_get_tx_queue has over 300 callers, most of these calls use the returned queue immediately without any checking on the returned value. I don't expect all of these callers to go and add conditionals to handle this case either. So I opted for the warning message and a valid return value. That being said, I am open to more opinions. > Piotr. >> + } >> + >> return &dev->_tx[index]; >> } >> >> -- >> 2.31.1 >>
On Fri, 17 Mar 2023 13:19:41 -0500 Nick Child wrote: > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 23b0d7eaaadd..fe88b1a7393d 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -2482,6 +2482,13 @@ static inline > struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev, > unsigned int index) > { > + if (unlikely(index >= dev->num_tx_queues)) { > + net_warn_ratelimited("%s selects TX queue %d, but number of TX queues is %d\n", > + dev->name, index, > + dev->num_tx_queues); > + return &dev->_tx[0]; > + } > + Should we maybe do DEBUG_NET_WARN_ON_ONCE() instead? It will likely run multiple times per each Tx packet, so I wonder if we really want to add a branch for what's effectively defensive programming...
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 23b0d7eaaadd..fe88b1a7393d 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2482,6 +2482,13 @@ static inline struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev, unsigned int index) { + if (unlikely(index >= dev->num_tx_queues)) { + net_warn_ratelimited("%s selects TX queue %d, but number of TX queues is %d\n", + dev->name, index, + dev->num_tx_queues); + return &dev->_tx[0]; + } + return &dev->_tx[index]; }
When requesting a TX queue at a given index, prevent out-of-bounds referencing by ensuring that the index is within the allocated number of queues. If there is an out-of-bounds reference then inform the user and return a reference to the first tx queue instead. Fixes: e8a0464cc950 ("netdev: Allocate multiple queues for TX.") Signed-off-by: Nick Child <nnac123@linux.ibm.com> --- include/linux/netdevice.h | 7 +++++++ 1 file changed, 7 insertions(+)