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]

Message-ID: <PH0PR11MB5207C23E220FD910DB99BE45F1F79@PH0PR11MB5207.namprd11.prod.outlook.com>
Date:   Tue, 10 Aug 2021 01:20:34 +0000
From:   "Nambiar, Amritha" <amritha.nambiar@...el.com>
To:     Alexander Duyck <alexander.duyck@...il.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

> -----Original Message-----
> From: Alexander Duyck <alexander.duyck@...il.com>
> Sent: Monday, August 9, 2021 5:51 PM
> To: Nambiar, Amritha <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.

Hi Alex,
Thanks for the review. The goal is to select a transmit queue using tc egress rule
and the action skbedit (that will go through netdev_pick_tx and skb_tx_hash).
I am not sure of the correct syntax for the queue-mapping value in the
action (tx-N or tx-N+1). As per the man page
(https://man7.org/linux/man-pages/man8/tc-skbedit.8.html), I interpreted
it as "action skbedit queue_mapping N" will transmit to tx-N. But, the
3 observations I listed don't quite seem to be following the tc rule.
Hence, tried to fix this in the action module. 

-Amritha