netdev - Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit

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: <a1716a3accbbf9a72bbedb011862ac57eaddf481.camel@redhat.com>
Date:   Wed, 20 Jun 2018 19:02:41 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Michel Machado <michel@...irati.com.br>,
        "Fu, Qiaobin" <qiaobinf@...edu>,
        "davem@...emloft.net" <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>
Subject: Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to
 skbedit

On Wed, 2018-06-20 at 12:47 -0400, Michel Machado wrote:
> On 06/20/2018 12:08 PM, Davide Caratti wrote:
> > On Tue, 2018-06-12 at 15:42 +0000, Fu, Qiaobin wrote:
> > > The new action inheritdsfield copies the field DS of
> > > IPv4 and IPv6 packets into skb->priority. This enables
> > > later classification of packets based on the DS field.
> > > 
> > > v4:
> > > *Not allow setting flags other than the expected ones.
> > > 
> > > *Allow dumping the pure flags.
> > > 
> > > Original idea by Jamal Hadi Salim <jhs@...atatu.com>
> > > 
> > > Signed-off-by: Qiaobin Fu <qiaobinf@...edu>
> > > Reviewed-by: Michel Machado <michel@...irati.com.br>
> > > ---
> > > 
> > 
> > [...]
> > 
> > > @@ -41,6 +44,25 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
> > >   
> > >   	if (d->flags & SKBEDIT_F_PRIORITY)
> > >   		skb->priority = d->priority;
> > > +	if (d->flags & SKBEDIT_F_INHERITDSFIELD) {
> > > +		int wlen = skb_network_offset(skb);
> > > +
> > > +		switch (tc_skb_protocol(skb)) {
> > > +		case htons(ETH_P_IP):
> > > +			wlen += sizeof(struct iphdr);
> > > +			if (!pskb_may_pull(skb, wlen))
> > > +				goto err;
> > > +			skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
> > > +			break;
> > > +
> > > +		case htons(ETH_P_IPV6):
> > > +			wlen += sizeof(struct ipv6hdr);
> > > +			if (!pskb_may_pull(skb, wlen))
> > > +				goto err;
> > > +			skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
> > > +			break;
> > > +		}
> > > +	}
> > >   	if (d->flags & SKBEDIT_F_QUEUE_MAPPING &&
> > >   	    skb->dev->real_num_tx_queues > d->queue_mapping)
> > >   		skb_set_queue_mapping(skb, d->queue_mapping);
> > > @@ -53,6 +75,10 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
> > >   
> > >   	spin_unlock(&d->tcf_lock);
> > >   	return d->tcf_action;
> > > +
> > > +err:
> > > +	spin_unlock(&d->tcf_lock);
> > > +	return TC_ACT_SHOT;
> > >   }
> > >   
> > 
> > sorry for asking this when the patch is a v4...
> > 
> > I spotted this, as I'm rebasing a small series that removes the tcf_lock
> > from the data plane of skbedit to gain some speed, and it converts the
> > stats to be per-cpu counters.
> > 
> > in the code above, you are catching failures of pskb_may_pull(skb) and
> > then you return TC_ACT_SHOT. That's OK, but I think you should update the
> > drop counter, like other TC actions do.
> > 
> > If you (author / reviewers) think this is a minor issue, like I do think,
> > then I can add the missing update in my series and post it when net-next
> > reopens.
> > 
> > WDYT?
> > 
> > thank you in advance!
> > regards,
> > 
> 
> Hi Davide,
> 
>     I agree that we should update the drop counter, but given that 
> you're already converting the stats to be per-cpu counters, whatever we 
> add now will be just symbolic since you're going to change it anyway.

that's ok for me also, as I can use the current v4 code for the rebase
(and not wait for another respin) _ but let's hear what reviewers think.

>  If 
> reviewers think that Qiaobin's patch must add the update line, could you 
> provide the exact line and location so we avoid going to v6 of this patch?

In case, I was thinking of something like: 

https://elixir.bootlin.com/linux/v4.18-rc1/source/net/sched/act_ipt.c#L249

so, between 'err:' and 'spin_unlock(&d->tcf_lock)', insert a line like:

d->tcf_qstats.drop++;

regards,
-- 
davide