netdev - Re: [net-next PATCH] net: act_skbedit: Fix tc action skbedit queue

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:   Mon, 9 Aug 2021 17:51:10 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Amritha Nambiar <amritha.nambiar@...el.com>
Cc:     Netdev <netdev@...r.kernel.org>, Jakub Kicinski <kuba@...nel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>,
        "Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Subject: Re: [net-next PATCH] net: act_skbedit: Fix tc action skbedit queue_mapping

On Mon, Aug 9, 2021 at 4:36 PM Amritha Nambiar
<amritha.nambiar@...el.com> wrote:
>
> For skbedit action queue_mapping to select the transmit queue,
> queue_mapping takes the value N for tx-N (where N is the actual
> queue number). However, current behavior is the following:
> 1. Queue selection is off by 1, tx queue N-1 is selected for
>    action skbedit queue_mapping N. (If the general syntax for queue
>    index is 1 based, i.e., action skbedit queue_mapping N would
>    transmit to tx queue N-1, where N >=1, then the last queue cannot
>    be used for transmit as this fails the upper bound check.)
> 2. Transmit to first queue of TCs other than TC0 selects the
>    next queue.
> 3. It is not possible to transmit to the first queue (tx-0) as
>    this fails the bounds check, in this case the fallback
>    mechanism for hash calculation is used.
>
> Fix the call to skb_set_queue_mapping(), the code retrieving the
> transmit queue uses skb_get_rx_queue() which subtracts the queue
> index by 1. This makes it so that "action skbedit queue_mapping N"
> will transmit to tx-N (including the first and last queue).
>
> Signed-off-by: Amritha Nambiar <amritha.nambiar@...el.com>
> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@...el.com>
> ---
>  net/sched/act_skbedit.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index e5f3fb8b00e3..a7bba15c74c4 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -59,7 +59,7 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a,
>         }
>         if (params->flags & SKBEDIT_F_QUEUE_MAPPING &&
>             skb->dev->real_num_tx_queues > params->queue_mapping)
> -               skb_set_queue_mapping(skb, params->queue_mapping);
> +               skb_set_queue_mapping(skb, params->queue_mapping + 1);
>         if (params->flags & SKBEDIT_F_MARK) {
>                 skb->mark &= ~params->mask;
>                 skb->mark |= params->mark & params->mask;
>

I don't think this is correct. It is conflating the rx_queue_mapping
versus the Tx queue mapping. This is supposed to be setting the Tx
queue mapping which applies after we have dropped the Rx queue
mapping, not before. Specifically this is run at the qdisc enqueue
stage with a single locked qdisc, after netdev_pick_tx and skb_tx_hash
have already run. It is something that existed before mq and is meant
to work with the mutliq qdisc.

If you are wanting to add a seperate override to add support for
programming the Rx queue mapping you may want to submit that as a
different patch rather than trying to change the existing Tx queue
mapping feature. Either that or you would need to change this so that
it has a different behavior depending on where the hook is added since
the behavior would be different if this is called before skb_tx_hash.