On 2020/9/11 4:07, Cong Wang wrote:
> On Tue, Sep 8, 2020 at 4:06 AM Yunsheng Lin <linyunsh...@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 <linyunsh...@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.
It seems so.
Maybe another patch to remove qdisc_lock(qdisc) for lockless
qdisc?
>
>> +
>> + 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.
The reference to the txq->qdisc is always protected by RCU, so the
synchronize_net()
should be enought to ensure there is no skb enqueued to the old qdisc that is
saved
in the dev_queue->qdisc_sleeping, because __dev_queue_xmit can only see the new
qdisc
after synchronize_net(), which is noop_qdisc, and noop_qdisc will make sure any
skb
enqueued to it will be dropped and freed, right?
If we do any additional reset that is not related to qdisc in
dev_reset_queue(), we
can move it after some_qdisc_is_busy() checking.
Also, it seems the __QDISC_STATE_DEACTIVATED checking in qdisc_run() is
unnecessary
after this patch, because after synchronize_net() qdisc_run() will now see the
old
qdisc.
static inline void qdisc_run(struct Qdisc *q)
{
if (qdisc_run_begin(q)) {
/* NOLOCK qdisc must check 'state' under the qdisc seqlock
* to avoid racing with dev_qdisc_reset()
*/
if (!(q->flags & TCQ_F_NOLOCK) ||
likely(!test_bit(__QDISC_STATE_DEACTIVATED, &q->state)))
__qdisc_run(q);
qdisc_run_end(q);
}
}
>
> Thanks.
> .
>
