netdev - Re: [PATCH net] net: sched: check netif_xmit_frozen_or_stopped() in sch_direct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPhsuW53SQCrWX-y-Ewcs5GS80NrsF_skpiv7Qwvb7fWy72Cgg@mail.gmail.com>
Date:   Fri, 25 May 2018 12:46:50 -0700
From:   Song Liu <liu.song.a23@...il.com>
To:     Song Liu <songliubraving@...com>, ast@...com
Cc:     netdev@...r.kernel.org, kernel-team@...com,
        John Fastabend <john.fastabend@...il.com>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] net: sched: check netif_xmit_frozen_or_stopped() in sch_direct_xmit()

On Fri, May 25, 2018 at 11:11 AM, Song Liu <songliubraving@...com> wrote:
> Summary:
>
> At the end of sch_direct_xmit(), we are in the else path of
> !dev_xmit_complete(ret), which means ret == NETDEV_TX_OK. The following
> condition will always fail and netif_xmit_frozen_or_stopped() is not
> checked at all.
>
>     if (ret && netif_xmit_frozen_or_stopped(txq))
>          return false;
>
> In this patch, this condition is fixed as:
>
>     if (netif_xmit_frozen_or_stopped(txq))
>          return false;
>
> and further simplifies the code as:
>
>     return !netif_xmit_frozen_or_stopped(txq);
>
> Fixes: 29b86cdac00a ("net: sched: remove remaining uses for qdisc_qlen in xmit path")
> Cc: John Fastabend <john.fastabend@...il.com>
> Cc: David S. Miller <davem@...emloft.net>
> Signed-off-by: Song Liu <songliubraving@...com>
> ---
>  net/sched/sch_generic.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 39c144b..8261d48 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -346,10 +346,7 @@ bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
>                 return false;
>         }
>
> -       if (ret && netif_xmit_frozen_or_stopped(txq))
> -               return false;
> -
> -       return true;
> +       return !netif_xmit_frozen_or_stopped(txq);
>  }
>
>  /*
> --
> 2.9.5
>

Alexei and I discussed about this offline. We would like to share our
discussion here to
clarify the motivation.

Before 29b86cdac00a, ret in condition "if (ret &&
netif_xmit_frozen_or_stopped()" is not
the value from dev_hard_start_xmit(), because ret is overwritten by
either qdisc_qlen()
or dev_requeue_skb(). Therefore, 29b86cdac00a changed the behavior of
this condition.

For ret from dev_hard_start_xmit(), I dig into the function and found
it is from return value
of ndo_start_xmit(). Per netdevice.h, ndo_start_xmit() should only
return NETDEV_TX_OK
or NETDEV_TX_BUSY. I survey many drivers, and they all follow the rule. The only
exception is vlan.

Given ret could only be NETDEV_TX_OK or NETDEV_TX_BUSY (ignore vlan for now),
if it fails condition "if (!dev_xmit_complete(ret))", ret must be
NETDEV_TX_OK == 0. So
netif_xmit_frozen_or_stopped() will always be bypassed.

It is probably OK to ignore netif_xmit_frozen_or_stopped(), and return true from
sch_direct_xmit(), as I didn't see that break any functionality. But
it is more like "correct
by accident" to me. This is the motivation of my original patch.

Alexei pointed out that, the following condition is more like original logic:

      if (qdisc_qlen(q) && netif_xmit_frozen_or_stopped(txq))
            return false;

However, I think John would like to remove qdisc_qlen() from the tx
path. I didn't see
any issue without the extra qdisc_qlen() check, so the patch is
probably good AS-IS.

Please share your comments and feedback on this.

Thanks,
Song