A failure in validate_xmit_skb_list() triggered an unconditional call
to dev_requeue_skb with skb=NULL. This slowly grows the queue
discipline's qlen count until all traffic through the queue stops.
By introducing a NULL check in dev_requeue_skb it was also necessary
to make the __netif_schedule call conditional to avoid scheduling an
empty queue.
Fixes: 55a93b3ea780 ("qdisc: validate skb without holding lock")
Signed-off-by: Lars Persson <[email protected]>
---
net/sched/sch_generic.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index f18c350..4e6a79c 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -47,10 +47,13 @@ EXPORT_SYMBOL(default_qdisc_ops);
static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
{
- q->gso_skb = skb;
- q->qstats.requeues++;
- q->q.qlen++; /* it's still part of the queue */
- __netif_schedule(q);
+ if (skb) {
+ q->gso_skb = skb;
+ q->qstats.requeues++;
+ q->q.qlen++; /* it's still part of the queue */
+ }
+ if (qdisc_qlen(q))
+ __netif_schedule(q);
return 0;
}
--
2.1.4
On Mon, 2016-04-11 at 08:24 +0200, Lars Persson wrote:
> A failure in validate_xmit_skb_list() triggered an unconditional call
> to dev_requeue_skb with skb=NULL. This slowly grows the queue
> discipline's qlen count until all traffic through the queue stops.
>
> By introducing a NULL check in dev_requeue_skb it was also necessary
> to make the __netif_schedule call conditional to avoid scheduling an
> empty queue.
>
> Fixes: 55a93b3ea780 ("qdisc: validate skb without holding lock")
> Signed-off-by: Lars Persson <[email protected]>
> ---
> net/sched/sch_generic.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index f18c350..4e6a79c 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -47,10 +47,13 @@ EXPORT_SYMBOL(default_qdisc_ops);
>
> static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
> {
> - q->gso_skb = skb;
> - q->qstats.requeues++;
> - q->q.qlen++; /* it's still part of the queue */
> - __netif_schedule(q);
> + if (skb) {
> + q->gso_skb = skb;
> + q->qstats.requeues++;
> + q->q.qlen++; /* it's still part of the queue */
> + }
> + if (qdisc_qlen(q))
> + __netif_schedule(q);
>
> return 0;
> }
Please always CC patch author when fixing a bug.
Why adding the if (qdisc_qlen(q)) extra test ?
This seems unrelated to the bug fix, and probably should be part of a
second patch targeting net-next tree.
Also please add a likely() clause
if (likely(skb)) {
q->gso_skb = skb;
q->qstats.requeues++;
q->q.qlen++; /* it's still part of the queue */
__netif_schedule(q);
}
Thanks !
On 04/11/2016 03:23 PM, Eric Dumazet wrote:
> On Mon, 2016-04-11 at 08:24 +0200, Lars Persson wrote:
>> A failure in validate_xmit_skb_list() triggered an unconditional call
>> to dev_requeue_skb with skb=NULL. This slowly grows the queue
>> discipline's qlen count until all traffic through the queue stops.
>>
>> By introducing a NULL check in dev_requeue_skb it was also necessary
>> to make the __netif_schedule call conditional to avoid scheduling an
>> empty queue.
>>
>> Fixes: 55a93b3ea780 ("qdisc: validate skb without holding lock")
>> Signed-off-by: Lars Persson <[email protected]>
>> ---
>> net/sched/sch_generic.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>> index f18c350..4e6a79c 100644
>> --- a/net/sched/sch_generic.c
>> +++ b/net/sched/sch_generic.c
>> @@ -47,10 +47,13 @@ EXPORT_SYMBOL(default_qdisc_ops);
>>
>> static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
>> {
>> - q->gso_skb = skb;
>> - q->qstats.requeues++;
>> - q->q.qlen++; /* it's still part of the queue */
>> - __netif_schedule(q);
>> + if (skb) {
>> + q->gso_skb = skb;
>> + q->qstats.requeues++;
>> + q->q.qlen++; /* it's still part of the queue */
>> + }
>> + if (qdisc_qlen(q))
>> + __netif_schedule(q);
>>
>> return 0;
>> }
>
>
> Please always CC patch author when fixing a bug.
>
> Why adding the if (qdisc_qlen(q)) extra test ?
>
> This seems unrelated to the bug fix, and probably should be part of a
> second patch targeting net-next tree.
I though it would be prudent because the queue can be non-empty even for
the case of skb=NULL. So should it be there in this patch, another patch
or not at all ?
>
> Also please add a likely() clause
>
> if (likely(skb)) {
> q->gso_skb = skb;
> q->qstats.requeues++;
> q->q.qlen++; /* it's still part of the queue */
> __netif_schedule(q);
> }
Will fix.
> Thanks !
>
>
>
>
>
On Mon, 2016-04-11 at 15:38 +0200, Lars Persson wrote:
> I though it would be prudent because the queue can be non-empty even for
> the case of skb=NULL. So should it be there in this patch, another patch
> or not at all ?
Then maybe change return code ?
It seems strange that a validate_xmit_skb_list() failure stops the
__qdisc_run() loop but schedules another round.
On 04/11/2016 04:22 PM, Eric Dumazet wrote:
> On Mon, 2016-04-11 at 15:38 +0200, Lars Persson wrote:
>
>> I though it would be prudent because the queue can be non-empty even for
>> the case of skb=NULL. So should it be there in this patch, another patch
>> or not at all ?
>
> Then maybe change return code ?
>
> It seems strange that a validate_xmit_skb_list() failure stops the
> __qdisc_run() loop but schedules another round.
>
>
It was suggested by Cong Wang to return 0 in order to stop the loop. Do
you guys agree that the loop should be stopped for such failures ? Then
I will put the schedule call inside the if as you proposed earlier.
- Lars
On Mon, 2016-04-11 at 17:17 +0200, Lars Persson wrote:
>
> On 04/11/2016 04:22 PM, Eric Dumazet wrote:
> > On Mon, 2016-04-11 at 15:38 +0200, Lars Persson wrote:
> >
> >> I though it would be prudent because the queue can be non-empty even for
> >> the case of skb=NULL. So should it be there in this patch, another patch
> >> or not at all ?
> >
> > Then maybe change return code ?
> >
> > It seems strange that a validate_xmit_skb_list() failure stops the
> > __qdisc_run() loop but schedules another round.
> >
> >
>
> It was suggested by Cong Wang to return 0 in order to stop the loop. Do
> you guys agree that the loop should be stopped for such failures ? Then
> I will put the schedule call inside the if as you proposed earlier.
What are the causes of validate_xmit_skb_list() failures ?
If gso segmentations fail because of memory pressure, better free more
skbs right now.
In any case, having a single test " if (skb) " sounds better to me,
to have a fast path.
So your first patch was probably a better idea.
v2 has two tests instead of one.
On Mon, Apr 11, 2016 at 8:17 AM, Lars Persson <[email protected]> wrote:
>
>
> On 04/11/2016 04:22 PM, Eric Dumazet wrote:
>>
>> On Mon, 2016-04-11 at 15:38 +0200, Lars Persson wrote:
>>
>>> I though it would be prudent because the queue can be non-empty even for
>>> the case of skb=NULL. So should it be there in this patch, another patch
>>> or not at all ?
>>
>>
>> Then maybe change return code ?
>>
>> It seems strange that a validate_xmit_skb_list() failure stops the
>> __qdisc_run() loop but schedules another round.
>>
>>
>
> It was suggested by Cong Wang to return 0 in order to stop the loop. Do you
> guys agree that the loop should be stopped for such failures ? Then I will
> put the schedule call inside the if as you proposed earlier.
I don't see any reason why we should continue on failure. And, I didn't
suggest you to return reschedule it either. I was suggesting to just return
0 for skb == NULL case.
On Mon, Apr 11, 2016 at 8:52 AM, Eric Dumazet <[email protected]> wrote:
> On Mon, 2016-04-11 at 17:17 +0200, Lars Persson wrote:
>>
>> On 04/11/2016 04:22 PM, Eric Dumazet wrote:
>> > On Mon, 2016-04-11 at 15:38 +0200, Lars Persson wrote:
>> >
>> >> I though it would be prudent because the queue can be non-empty even for
>> >> the case of skb=NULL. So should it be there in this patch, another patch
>> >> or not at all ?
>> >
>> > Then maybe change return code ?
>> >
>> > It seems strange that a validate_xmit_skb_list() failure stops the
>> > __qdisc_run() loop but schedules another round.
>> >
>> >
>>
>> It was suggested by Cong Wang to return 0 in order to stop the loop. Do
>> you guys agree that the loop should be stopped for such failures ? Then
>> I will put the schedule call inside the if as you proposed earlier.
>
> What are the causes of validate_xmit_skb_list() failures ?
>
> If gso segmentations fail because of memory pressure, better free more
> skbs right now.
>
> In any case, having a single test " if (skb) " sounds better to me,
> to have a fast path.
>
> So your first patch was probably a better idea.
>
> v2 has two tests instead of one.
I am fine with either way as long as the loop stops on failure.
Folding the test "if (skb)" into one also requires to retake the spinlock.
On Mon, 2016-04-11 at 11:02 -0700, Cong Wang wrote:
> I am fine with either way as long as the loop stops on failure.
> Folding the test "if (skb)" into one also requires to retake the spinlock.
Adding the likely() in this path would probably help as well.
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index f18c35024207..07202d9ac4f6 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -159,12 +159,14 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
if (validate)
skb = validate_xmit_skb_list(skb, dev);
- if (skb) {
+ if (likely(skb)) {
HARD_TX_LOCK(dev, txq, smp_processor_id());
if (!netif_xmit_frozen_or_stopped(txq))
skb = dev_hard_start_xmit(skb, dev, txq, &ret);
HARD_TX_UNLOCK(dev, txq);
+ } else {
+ ... does all and return...
}
spin_lock(root_lock);
On Mon, 2016-04-11 at 11:26 -0700, Eric Dumazet wrote:
> On Mon, 2016-04-11 at 11:02 -0700, Cong Wang wrote:
>
> > I am fine with either way as long as the loop stops on failure.
Note that skb that could not be validated is already freed.
So I do not see any value from stopping the loop, since
we need to schedule the queue to avoid tx hang.
Just process following skb if there is one, fact that skb is sent or
dropped does not matter.
On Mon, Apr 11, 2016 at 11:30 AM, Eric Dumazet <[email protected]> wrote:
> On Mon, 2016-04-11 at 11:26 -0700, Eric Dumazet wrote:
>> On Mon, 2016-04-11 at 11:02 -0700, Cong Wang wrote:
>>
>> > I am fine with either way as long as the loop stops on failure.
>
>
> Note that skb that could not be validated is already freed.
>
> So I do not see any value from stopping the loop, since
> we need to schedule the queue to avoid tx hang.
>
> Just process following skb if there is one, fact that skb is sent or
> dropped does not matter.
My point is, for example, in OOM case, we don't know processing
more SKB would make it better or worse. Maybe we really need to
check the error code to decide to continue to exit?
On Mon, 2016-04-11 at 16:19 -0700, Cong Wang wrote:
> My point is, for example, in OOM case, we don't know processin
> more SKB would make it better or worse. Maybe we really need to
> check the error code to decide to continue to exit?
Really, given this bug has been there for a long time (v3.18 ???), I
doubt it matters.
Nothing can tell that following packets in the qdisc need any
transformation, and memory allocations.
So I would just fix the bug in the simplest way.
__qdisc_run() has all the checks needed to yield when needed
(if (quota <= 0 || need_resched())) , no need to add more complexity
there.