[PATCH net-next] net: sched: revert tx_queue_len on partial qdisc resize failure

[PATCH net-next] net: sched: revert tx_queue_len on partial qdisc resize failure

[Adriano Cordova]

Pass the old length to dev_qdisc_change_tx_queue_len() and, when a queue
fails to resize, use it to restore the queues already handled.

Fixes: 48bfd55e7e41 ("net_sched: plug in qdisc ops change_tx_queue_len")
Signed-off-by: Adriano Cordova <adrianox@gmail.com>
---
 include/net/sch_generic.h |  2 +-
 net/core/dev.c            |  2 +-
 net/sched/sch_generic.c   | 15 +++++++++------
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index f35bd06a6bad..0ce97b4b2da1 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -720,7 +720,7 @@ void qdisc_class_hash_remove(struct Qdisc_class_hash *,
 void qdisc_class_hash_grow(struct Qdisc *, struct Qdisc_class_hash *);
 void qdisc_class_hash_destroy(struct Qdisc_class_hash *);
 
-int dev_qdisc_change_tx_queue_len(struct net_device *dev);
+int dev_qdisc_change_tx_queue_len(struct net_device *dev, unsigned int old_len);
 void dev_qdisc_change_real_num_tx(struct net_device *dev,
 				  unsigned int new_real_tx);
 void dev_init_scheduler(struct net_device *dev);
diff --git a/net/core/dev.c b/net/core/dev.c
index f660fccfc0db..57e1fa320d53 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10002,7 +10002,7 @@ int netif_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
 		res = notifier_to_errno(res);
 		if (res)
 			goto err_rollback;
-		res = dev_qdisc_change_tx_queue_len(dev);
+		res = dev_qdisc_change_tx_queue_len(dev, orig_len);
 		if (res)
 			goto err_rollback;
 	}
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 6f6a6f0d5eb0..8fd0fce23cc4 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1422,13 +1422,14 @@ void dev_deactivate(struct net_device *dev, bool reset_needed)
 EXPORT_SYMBOL(dev_deactivate);
 
 static int qdisc_change_tx_queue_len(struct net_device *dev,
-				     struct netdev_queue *dev_queue)
+				     struct netdev_queue *dev_queue,
+				     unsigned int len)
 {
 	struct Qdisc *qdisc = rtnl_dereference(dev_queue->qdisc_sleeping);
 	const struct Qdisc_ops *ops = qdisc->ops;
 
 	if (ops->change_tx_queue_len)
-		return ops->change_tx_queue_len(qdisc, dev->tx_queue_len);
+		return ops->change_tx_queue_len(qdisc, len);
 	return 0;
 }
 
@@ -1465,7 +1466,7 @@ void mq_change_real_num_tx(struct Qdisc *sch, unsigned int new_real_tx)
 }
 EXPORT_SYMBOL(mq_change_real_num_tx);
 
-int dev_qdisc_change_tx_queue_len(struct net_device *dev)
+int dev_qdisc_change_tx_queue_len(struct net_device *dev, unsigned int old_len)
 {
 	bool up = dev->flags & IFF_UP;
 	unsigned int i;
@@ -1475,13 +1476,15 @@ int dev_qdisc_change_tx_queue_len(struct net_device *dev)
 		dev_deactivate(dev, false);
 
 	for (i = 0; i < dev->num_tx_queues; i++) {
-		ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i]);
-
-		/* TODO: revert changes on a partial failure */
+		ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i],
+						dev->tx_queue_len);
 		if (ret)
 			break;
 	}
 
+	while (ret && i--)
+		qdisc_change_tx_queue_len(dev, &dev->_tx[i], old_len);
+
 	if (up)
 		dev_activate(dev);
 	return ret;
-- 
2.51.0

Re: [PATCH net-next] net: sched: revert tx_queue_len on partial qdisc resize failure

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net-next] net: sched: revert tx_queue_len on partial qdisc resize failure

[Eric Dumazet]

On Mon, Sep 28, 2026 at 4:22 PM Adriano Cordova <adrianox@gmail.com> wrote:
>
> Pass the old length to dev_qdisc_change_tx_queue_len() and, when a queue
> fails to resize, use it to restore the queues already handled.
>
> Fixes: 48bfd55e7e41 ("net_sched: plug in qdisc ops change_tx_queue_len")
> Signed-off-by: Adriano Cordova <adrianox@gmail.com>
> ---
>  include/net/sch_generic.h |  2 +-
>  net/core/dev.c            |  2 +-
>  net/sched/sch_generic.c   | 15 +++++++++------
>  3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index f35bd06a6bad..0ce97b4b2da1 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -720,7 +720,7 @@ void qdisc_class_hash_remove(struct Qdisc_class_hash *,
>  void qdisc_class_hash_grow(struct Qdisc *, struct Qdisc_class_hash *);
>  void qdisc_class_hash_destroy(struct Qdisc_class_hash *);
>
> -int dev_qdisc_change_tx_queue_len(struct net_device *dev);
> +int dev_qdisc_change_tx_queue_len(struct net_device *dev, unsigned int old_len);
>  void dev_qdisc_change_real_num_tx(struct net_device *dev,
>                                   unsigned int new_real_tx);
>  void dev_init_scheduler(struct net_device *dev);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index f660fccfc0db..57e1fa320d53 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10002,7 +10002,7 @@ int netif_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
>                 res = notifier_to_errno(res);
>                 if (res)
>                         goto err_rollback;
> -               res = dev_qdisc_change_tx_queue_len(dev);
> +               res = dev_qdisc_change_tx_queue_len(dev, orig_len);
>                 if (res)
>                         goto err_rollback;
>         }
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 6f6a6f0d5eb0..8fd0fce23cc4 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1422,13 +1422,14 @@ void dev_deactivate(struct net_device *dev, bool reset_needed)
>  EXPORT_SYMBOL(dev_deactivate);
>
>  static int qdisc_change_tx_queue_len(struct net_device *dev,
> -                                    struct netdev_queue *dev_queue)
> +                                    struct netdev_queue *dev_queue,
> +                                    unsigned int len)
>  {
>         struct Qdisc *qdisc = rtnl_dereference(dev_queue->qdisc_sleeping);
>         const struct Qdisc_ops *ops = qdisc->ops;
>
>         if (ops->change_tx_queue_len)
> -               return ops->change_tx_queue_len(qdisc, dev->tx_queue_len);
> +               return ops->change_tx_queue_len(qdisc, len);
>         return 0;
>  }
>
> @@ -1465,7 +1466,7 @@ void mq_change_real_num_tx(struct Qdisc *sch, unsigned int new_real_tx)
>  }
>  EXPORT_SYMBOL(mq_change_real_num_tx);
>
> -int dev_qdisc_change_tx_queue_len(struct net_device *dev)
> +int dev_qdisc_change_tx_queue_len(struct net_device *dev, unsigned int old_len)
>  {
>         bool up = dev->flags & IFF_UP;
>         unsigned int i;
> @@ -1475,13 +1476,15 @@ int dev_qdisc_change_tx_queue_len(struct net_device *dev)
>                 dev_deactivate(dev, false);
>
>         for (i = 0; i < dev->num_tx_queues; i++) {
> -               ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i]);
> -
> -               /* TODO: revert changes on a partial failure */
> +               ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i],
> +                                               dev->tx_queue_len);
>                 if (ret)
>                         break;
>         }
>
> +       while (ret && i--)
> +               qdisc_change_tx_queue_len(dev, &dev->_tx[i], old_len);
> +

Ah... Probably driven by the TODO  comment and a LLM ?

One caveat is that pfifo_fast_change_tx_queue_len() is currently the
only ->change_tx_queue_len() implementation, and it only fails on
-ENOMEM (inside skb_array_resize_multiple_bh() after freeing the
previous ring arrays on already-processed queues).

Trying to revert queues 0..i-1 back to old_len will therefore have to
allocate new ptr_ring arrays right after an -ENOMEM failure (which is
especially likely to fail again if old_len > new_len), and cannot
recover packets already dropped if the ring was shrunk.

pw-bot: cr

Re: [PATCH net-next] net: sched: revert tx_queue_len on partial qdisc resize failure

[Adriano Córdova]

El lun, 28 sept 2026 a las 11:38, Eric Dumazet (<edumazet@google.com>) escribió:
>
> On Mon, Sep 28, 2026 at 4:22 PM Adriano Cordova <adrianox@gmail.com> wrote:
> >
> > Pass the old length to dev_qdisc_change_tx_queue_len() and, when a queue
> > fails to resize, use it to restore the queues already handled.
> >
> > Fixes: 48bfd55e7e41 ("net_sched: plug in qdisc ops change_tx_queue_len")
> > Signed-off-by: Adriano Cordova <adrianox@gmail.com>
> > ---
> >  include/net/sch_generic.h |  2 +-
> >  net/core/dev.c            |  2 +-
> >  net/sched/sch_generic.c   | 15 +++++++++------
> >  3 files changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> > index f35bd06a6bad..0ce97b4b2da1 100644
> > --- a/include/net/sch_generic.h
> > +++ b/include/net/sch_generic.h
> > @@ -720,7 +720,7 @@ void qdisc_class_hash_remove(struct Qdisc_class_hash *,
> >  void qdisc_class_hash_grow(struct Qdisc *, struct Qdisc_class_hash *);
> >  void qdisc_class_hash_destroy(struct Qdisc_class_hash *);
> >
> > -int dev_qdisc_change_tx_queue_len(struct net_device *dev);
> > +int dev_qdisc_change_tx_queue_len(struct net_device *dev, unsigned int old_len);
> >  void dev_qdisc_change_real_num_tx(struct net_device *dev,
> >                                   unsigned int new_real_tx);
> >  void dev_init_scheduler(struct net_device *dev);
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index f660fccfc0db..57e1fa320d53 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -10002,7 +10002,7 @@ int netif_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
> >                 res = notifier_to_errno(res);
> >                 if (res)
> >                         goto err_rollback;
> > -               res = dev_qdisc_change_tx_queue_len(dev);
> > +               res = dev_qdisc_change_tx_queue_len(dev, orig_len);
> >                 if (res)
> >                         goto err_rollback;
> >         }
> > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> > index 6f6a6f0d5eb0..8fd0fce23cc4 100644
> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -1422,13 +1422,14 @@ void dev_deactivate(struct net_device *dev, bool reset_needed)
> >  EXPORT_SYMBOL(dev_deactivate);
> >
> >  static int qdisc_change_tx_queue_len(struct net_device *dev,
> > -                                    struct netdev_queue *dev_queue)
> > +                                    struct netdev_queue *dev_queue,
> > +                                    unsigned int len)
> >  {
> >         struct Qdisc *qdisc = rtnl_dereference(dev_queue->qdisc_sleeping);
> >         const struct Qdisc_ops *ops = qdisc->ops;
> >
> >         if (ops->change_tx_queue_len)
> > -               return ops->change_tx_queue_len(qdisc, dev->tx_queue_len);
> > +               return ops->change_tx_queue_len(qdisc, len);
> >         return 0;
> >  }
> >
> > @@ -1465,7 +1466,7 @@ void mq_change_real_num_tx(struct Qdisc *sch, unsigned int new_real_tx)
> >  }
> >  EXPORT_SYMBOL(mq_change_real_num_tx);
> >
> > -int dev_qdisc_change_tx_queue_len(struct net_device *dev)
> > +int dev_qdisc_change_tx_queue_len(struct net_device *dev, unsigned int old_len)
> >  {
> >         bool up = dev->flags & IFF_UP;
> >         unsigned int i;
> > @@ -1475,13 +1476,15 @@ int dev_qdisc_change_tx_queue_len(struct net_device *dev)
> >                 dev_deactivate(dev, false);
> >
> >         for (i = 0; i < dev->num_tx_queues; i++) {
> > -               ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i]);
> > -
> > -               /* TODO: revert changes on a partial failure */
> > +               ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i],
> > +                                               dev->tx_queue_len);
> >                 if (ret)
> >                         break;
> >         }
> >
> > +       while (ret && i--)
> > +               qdisc_change_tx_queue_len(dev, &dev->_tx[i], old_len);
> > +
>
> Ah... Probably driven by the TODO  comment and a LLM ?
>
> One caveat is that pfifo_fast_change_tx_queue_len() is currently the
> only ->change_tx_queue_len() implementation, and it only fails on
> -ENOMEM (inside skb_array_resize_multiple_bh() after freeing the
> previous ring arrays on already-processed queues).
>
> Trying to revert queues 0..i-1 back to old_len will therefore have to
> allocate new ptr_ring arrays right after an -ENOMEM failure (which is
> especially likely to fail again if old_len > new_len), and cannot
> recover packets already dropped if the ring was shrunk.
>
> pw-bot: cr

Yes.

So there is not much to do about the TODO. I could send a v2 where
qdisc_change_tx_queue_len returns void and emits a warning on failure.