Messages in this thread | | | From | Cong Wang <> | Date | Thu, 10 Sep 2020 13:07:16 -0700 | Subject | Re: [PATCH v2 net] net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc |
| |
On Tue, Sep 8, 2020 at 4:06 AM Yunsheng Lin <linyunsheng@huawei.com> wrote: > > Currently there is concurrent reset and enqueue operation for the > same lockless qdisc when there is no lock to synchronize the > q->enqueue() in __dev_xmit_skb() with the qdisc reset operation in > qdisc_deactivate() called by dev_deactivate_queue(), which may cause > out-of-bounds access for priv->ring[] in hns3 driver if user has > requested a smaller queue num when __dev_xmit_skb() still enqueue a > skb with a larger queue_mapping after the corresponding qdisc is > reset, and call hns3_nic_net_xmit() with that skb later. > > Reused the existing synchronize_net() in dev_deactivate_many() to > make sure skb with larger queue_mapping enqueued to old qdisc(which > is saved in dev_queue->qdisc_sleeping) will always be reset when > dev_reset_queue() is called. > > Fixes: 6b3ba9146fe6 ("net: sched: allow qdiscs to handle locking") > Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com> > --- > ChangeLog V2: > Reuse existing synchronize_net(). > --- > net/sched/sch_generic.c | 48 +++++++++++++++++++++++++++++++++--------------- > 1 file changed, 33 insertions(+), 15 deletions(-) > > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c > index 265a61d..54c4172 100644 > --- a/net/sched/sch_generic.c > +++ b/net/sched/sch_generic.c > @@ -1131,24 +1131,10 @@ EXPORT_SYMBOL(dev_activate); > > static void qdisc_deactivate(struct Qdisc *qdisc) > { > - bool nolock = qdisc->flags & TCQ_F_NOLOCK; > - > if (qdisc->flags & TCQ_F_BUILTIN) > return; > - if (test_bit(__QDISC_STATE_DEACTIVATED, &qdisc->state)) > - return; > - > - if (nolock) > - spin_lock_bh(&qdisc->seqlock); > - spin_lock_bh(qdisc_lock(qdisc)); > > set_bit(__QDISC_STATE_DEACTIVATED, &qdisc->state); > - > - qdisc_reset(qdisc); > - > - spin_unlock_bh(qdisc_lock(qdisc)); > - if (nolock) > - spin_unlock_bh(&qdisc->seqlock); > } > > static void dev_deactivate_queue(struct net_device *dev, > @@ -1165,6 +1151,30 @@ static void dev_deactivate_queue(struct net_device *dev, > } > } > > +static void dev_reset_queue(struct net_device *dev, > + struct netdev_queue *dev_queue, > + void *_unused) > +{ > + struct Qdisc *qdisc; > + bool nolock; > + > + qdisc = dev_queue->qdisc_sleeping; > + if (!qdisc) > + return; > + > + nolock = qdisc->flags & TCQ_F_NOLOCK; > + > + if (nolock) > + spin_lock_bh(&qdisc->seqlock); > + spin_lock_bh(qdisc_lock(qdisc));
I think you do not need this lock for lockless one.
> + > + qdisc_reset(qdisc); > + > + spin_unlock_bh(qdisc_lock(qdisc)); > + if (nolock) > + spin_unlock_bh(&qdisc->seqlock); > +} > + > static bool some_qdisc_is_busy(struct net_device *dev) > { > unsigned int i; > @@ -1213,12 +1223,20 @@ void dev_deactivate_many(struct list_head *head) > dev_watchdog_down(dev); > } > > - /* Wait for outstanding qdisc-less dev_queue_xmit calls. > + /* Wait for outstanding qdisc-less dev_queue_xmit calls or > + * outstanding qdisc enqueuing calls. > * This is avoided if all devices are in dismantle phase : > * Caller will call synchronize_net() for us > */ > synchronize_net(); > > + list_for_each_entry(dev, head, close_list) { > + netdev_for_each_tx_queue(dev, dev_reset_queue, NULL); > + > + if (dev_ingress_queue(dev)) > + dev_reset_queue(dev, dev_ingress_queue(dev), NULL); > + } > + > /* Wait for outstanding qdisc_run calls. */ > list_for_each_entry(dev, head, close_list) { > while (some_qdisc_is_busy(dev)) {
Do you want to reset before waiting for TX action?
I think it is safer to do it after, at least prior to commit 759ae57f1b we did after.
Thanks.
| |