Re: [PATCH] net: make sure skb_dst is valid before using

[Nicolas Dichtel <nicolas.dichtel@6wind.com>]

Le 24/01/2018 à 10:49, Roman Kapl a écrit :
> On 01/24/2018 10:16 AM, Xin Long wrote:
>> On Wed, Jan 24, 2018 at 6:42 AM, Roman Kapl <code@rkapl.cz> wrote:
>>> Tunnel devices often use skb_dst(skb)->ops, but ops are not implemented
>>> for metadata tunnel destinations. Use skb_valid_dst to check if skb_dst
>>> is a real (non-metadata) destination.
>>>
>>> Such packets can easily be crafted using tc tunnel_key actions or BPF
>>> and will crash the tunnels, as observed at least with sit, geneve and
>>> vxlan. Some of these tunnels are also intended to be used with metadata
>>> destinations, so this represents a loss of functionality.
>>>
>>> There might be more places with similar patterns, but sometimes it is
>>> hard to tell if metadata dst packets can reach them, so this patch is
>>> not exhaustive.
>> This patch is trying to fix a lot of places, and there may be more.
>> But all because of the lack of .update_pmtu or .neigh_lookup.
>> (dst_link_failure is safe, btw)
>>
>> Not sure if it will be better to add .update_pmtu, .neigh_lookup and
>> even .redirect for md_dst_ops, but just leave them empty ?
>> like fake_dst_ops in br_nf.
> That's what I was suggesting in the original mail. However, it might have slight
> impact on performance. Also it was not done when metadata destinations were
> originally added, was there any good reason?
>> Let's see other's opinions.
> Exactly.
I would prefer a patch that test if the handler is available. It would prevent
to have this bug again in the future. But I don't have a strong opinion about this.

But it would be nice to send a patch quickly, before the 4.15 release, to fix
this crash.


Thank you,
Nicolas