netdev - Re: [RFC PATCH 15/17] net: sched: pfifo_fast use skb

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]

Date:   Tue, 14 Nov 2017 18:34:31 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Eric Dumazet <eric.dumazet@...il.com>, make0818@...il.com,
        Network Development <netdev@...r.kernel.org>,
        Jiří Pírko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>
Subject: Re: [RFC PATCH 15/17] net: sched: pfifo_fast use skb_array

>  static struct sk_buff *pfifo_fast_peek(struct Qdisc *qdisc)
>  {
>         struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
> -       int band = bitmap2band[priv->bitmap];
> +       struct sk_buff *skb = NULL;
> +       int band;
>
> -       if (band >= 0) {
> -               struct qdisc_skb_head *qh = band2list(priv, band);
> +       for (band = 0; band < PFIFO_FAST_BANDS && !skb; band++) {
> +               struct skb_array *q = band2list(priv, band);
>
> -               return qh->head;
> +               skb = __skb_array_peek(q);
> +               if (!skb)
> +                       continue;
>         }

This explicit continue is not needed.

>  static void pfifo_fast_reset(struct Qdisc *qdisc)
>  {
> -       int prio;
> +       int i, band;
>         struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
>
> -       for (prio = 0; prio < PFIFO_FAST_BANDS; prio++)
> -               __qdisc_reset_queue(band2list(priv, prio));
> +       for (band = 0; band < PFIFO_FAST_BANDS; band++) {
> +               struct skb_array *q = band2list(priv, band);
> +               struct sk_buff *skb;
>
> -       priv->bitmap = 0;
> -       qdisc->qstats.backlog = 0;
> -       qdisc->q.qlen = 0;
> +               while ((skb = skb_array_consume_bh(q)) != NULL)
> +                       __skb_array_destroy_skb(skb);

Can use regular kfree_skb after dequeue. This skb_array specific
callback probably only exists to match the ptr_ring callback typedef.

>  static int pfifo_fast_dump(struct Qdisc *qdisc, struct sk_buff *skb)
> @@ -685,17 +688,40 @@ static int pfifo_fast_dump(struct Qdisc *qdisc, struct sk_buff *skb)
>
>  static int pfifo_fast_init(struct Qdisc *qdisc, struct nlattr *opt)
>  {
> -       int prio;
> +       unsigned int qlen = qdisc_dev(qdisc)->tx_queue_len;
>         struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
> +       int prio;
> +
> +       /* guard against zero length rings */
> +       if (!qlen)
> +               return -EINVAL;
>
> -       for (prio = 0; prio < PFIFO_FAST_BANDS; prio++)
> -               qdisc_skb_head_init(band2list(priv, prio));
> +       for (prio = 0; prio < PFIFO_FAST_BANDS; prio++) {
> +               struct skb_array *q = band2list(priv, prio);
> +               int err;
> +
> +               err = skb_array_init(q, qlen, GFP_KERNEL);
> +               if (err)
> +                       return -ENOMEM;

This relies on the caller calling ops->destroy on error to free partially
allocated state. For uninitialized bands, that calls spin_lock on an
uninitialized spinlock from skb_array_cleanup -> ptr_ring_cleanup ->
ptr_ring_consume.

> +       }
>
>         /* Can by-pass the queue discipline */
>         qdisc->flags |= TCQ_F_CAN_BYPASS;
>         return 0;
>  }
>
> +static void pfifo_fast_destroy(struct Qdisc *sch)
> +{
> +       struct pfifo_fast_priv *priv = qdisc_priv(sch);
> +       int prio;
> +
> +       for (prio = 0; prio < PFIFO_FAST_BANDS; prio++) {
> +               struct skb_array *q = band2list(priv, prio);
> +
> +               skb_array_cleanup(q);
> +       }
> +}