netdev - Re: [PATCH v3 net-next 2/2] tc: add 'needs

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: <55285038.3060806@plumgrid.com>
Date:	Fri, 10 Apr 2015 15:35:36 -0700
From:	Alexei Starovoitov <ast@...mgrid.com>
To:	Jamal Hadi Salim <jhs@...atatu.com>,
	David Miller <davem@...emloft.net>
CC:	daniel@...earbox.net, tgraf@...g.ch, jiri@...nulli.us,
	netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

On 4/10/15 5:48 AM, Jamal Hadi Salim wrote:
>
> That doesnt give you L2 at offset 0 for _some devices_ which is what you
> say you need. In some devices it adds a second L2 header.

ok. fair point. will add a check to make sure such ingress_l2 qdisc
doesn't get applied to devices without header_ops.
Same check is done by af_packet.

>
> To your "bugs" comments:
> - updating csum; earlier i said i was conflicted it being a
> useful "feature".
> You are repeating again that it is a bug. It is not.
> This action is intended to  mirror or redirect packets, period.

without updating skb->csum act_mirred is breaking csum for
checksum_complete devices. If these redirected skbs are eventually
seen by stack, the stack will drop them. I cannot use such
'feature', so I would need to find a way to do mirred without fixing
act_mirred. Fine.

> +        needs_l2 = q->flags & TCQ_F_INGRESS_L2;
> +        if (needs_l2) {
> ..
> ....
> +        if (needs_l2)
>
> That is executed for _every_ packet going after ingress qdisc.

and there is hugely expensive spin_lock/unlock around ingress.
Comparing to lock cmpxchg the cost of branch is a noise.
But fine, I'll find a way to optimize the whole thing without
adding extra branches.

> They have to know whether their code
> will run on ingress or egress and the type of device etc.
> The AT_XXX provides that signal and dev->type fills in the other gap.

True. The program authors may want to know whether the packet is seen
on ingress or egress and take different actions inside the program,
but forcing them to _always_ parse the packet differently because of
it is not acceptable.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html