On 2020/9/11 16:13, Yunsheng Lin wrote:
> 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.
now -> not
sorry for the typo.
>
> 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.
>> .
>>
> .
>
