netdev - Re: [PATCH net 0/2] lwtunnel: make it really work, for IPv4

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: <878u7w6dxh.fsf@x220.int.ebiederm.org>
Date:	Wed, 23 Sep 2015 16:09:14 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Jiri Benc <jbenc@...hat.com>
Cc:	Thomas Graf <tgraf@...g.ch>, netdev@...r.kernel.org,
	Roopa Prabhu <roopa@...ulusnetworks.com>
Subject: Re: [PATCH net 0/2] lwtunnel: make it really work, for IPv4

Jiri Benc <jbenc@...hat.com> writes:

> On Wed, 23 Sep 2015 12:42:30 -0500, Eric W. Biederman wrote:
[snip]
>> So perhaps instead of:
>> +	if (arp->ar_op == htons(ARPOP_REQUEST) && skb_metadata_dst(skb))
>> +		reply_dst = (struct dst_entry *)
>> +			    iptunnel_metadata_reply(skb_metadata_dst(skb),
>> +						    GFP_ATOMIC);
>> +
>> The code would be:
>> 	if (arp->ar_op == htons(AROP_REQUEST) && dev->ndo_reply_dst)
>> 		reply_dst = dev->ndo_reply_dst(skb, GFP_ATOMIC);
>
> This is something I intended to do originally. I also considered adding
> the callback into metadata_dst itself. However, it doesn't make much
> sense for the time being.

If you convert this into the equivalent for neighbour discovery where
the ingress metadata dst has already been lost it makes a lot of sense.

Which is the question you were asking that I have been replying to as
I think this all through.

[snip]

> If there's another user of metadata_dst in the future, this needs to be
> changed anyway. We can add such ndo callback (or some other kind of
> callback) then, if it turns out to be needed. For now, we don't need it
> and the changes to make metadata_dst usable for other things than IP
> tunnels are not suitable for net.git.

I think you were misunderstanding what I was suggesting.

I was suggesting an ndo callback that does not need an metadata dst on
the skb but has enough information between the network device and the
skb itself to properly construct a dst that can be used for arp and
ndisc type replies where you want to exactly reverse the path.

*Blink* You were targeting net.git with a feature enhancement????
I will just ignore that.

>> For any network that does interesting things with network level
>> identifiers below IP this seems like a general problem.  Further it
>> seems more desirable to only perform an allocation when necessary,
>> rather than for every packet, and two for the packets that actually
>> need replies.
>
> The metadata_dst are only allocated when requested. Look at e.g.
> vxlan_collect_metadata function. If they're requested, it means they are
> needed. And they certainly need to be allocated for ARP replies to such
> packets. I don't see what could be further optimized here. Seems to me
> that you assume that the sole purpose of why metadata_dst exist is ARP
> which is not the case.

What I was observing is that in general the only tunneled packets that
need an ingress metadata dst for a tunneled medium ethernet like medium
are arp and ndisc packets.  In other cases if you aren't doing something
exceptional like openvswitch the normal routing should be sufficient.

Which means a ndo_reply_dst method could remove the need in many cases
for an ingress metadata dst to need to be allocated.

Regardless a netdevice operation that digs into the packet and figures
out what is necessary for a reply seems like the clean way to make this
work for both arp and neighbour discovery.

Eric

--
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