netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: "David S. Miller" <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>
Subject: Re: [PATCH 2/3] ipv4/icmp: l3mdev: Perform icmp error route lookup on source device routing table
Date: Thu, 13 Aug 2020 09:12:58 -0400 (EDT)	[thread overview]
Message-ID: <1251597699.6518.1597324378911.JavaMail.zimbra@efficios.com> (raw)
In-Reply-To: <20200812.144332.2288214156822456254.davem@davemloft.net>

----- On Aug 12, 2020, at 5:43 PM, David S. Miller davem@davemloft.net wrote:

> From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date: Tue, 11 Aug 2020 15:50:02 -0400
> 
>> @@ -465,6 +465,7 @@ static struct rtable *icmp_route_lookup(struct net *net,
>>  					int type, int code,
>>  					struct icmp_bxm *param)
>>  {
>> +	struct net_device *route_lookup_dev = NULL;
>>  	struct rtable *rt, *rt2;
>>  	struct flowi4 fl4_dec;
>>  	int err;
>> @@ -479,7 +480,17 @@ static struct rtable *icmp_route_lookup(struct net *net,
>>  	fl4->flowi4_proto = IPPROTO_ICMP;
>>  	fl4->fl4_icmp_type = type;
>>  	fl4->fl4_icmp_code = code;
>> -	fl4->flowi4_oif = l3mdev_master_ifindex(skb_dst(skb_in)->dev);
>> +	/*
>> +	 * The device used for looking up which routing table to use is
>> +	 * preferably the source whenever it is set, which should ensure
>> +	 * the icmp error can be sent to the source host, else fallback
>> +	 * on the destination device.
>> +	 */
>> +	if (skb_in->dev)
>> +		route_lookup_dev = skb_in->dev;
>> +	else if (skb_dst(skb_in))
>> +		route_lookup_dev = skb_dst(skb_in)->dev;
>> +	fl4->flowi4_oif = l3mdev_master_ifindex(route_lookup_dev);
> 
> The caller of icmp_route_lookup() uses the opposite prioritization of
> devices for determining the network namespace to use:
> 
>	if (rt->dst.dev)
>		net = dev_net(rt->dst.dev);
>	else if (skb_in->dev)
>		net = dev_net(skb_in->dev);
>	else
>		goto out;
> 
> Do we have to reverse the ordering there too?

Looking at the history:

Originally dst.dev was used as network namespace for icmp errors:

dde1bc0e6f861 (Denis V. Lunev           2008-01-22 23:50:57 -0800  450)         net = rt->u.dst.dev->nd_net;

commit dde1bc0e6f86183bc095d0774cd109f4edf66ea2
Author: Denis V. Lunev <den@openvz.org>
Date:   Tue Jan 22 23:50:57 2008 -0800

    [NETNS]: Add namespace for ICMP replying code.
    
    All needed API is done, the namespace is available when required from
    the device on the DST entry from the incoming packet. So, just replace
    init_net with proper namespace.

Here I wonder what motivated use of the DST entry here ?

Note that this choice of DST network namespace applies to both __icmp_send and
icmp_unreach.

It has been followed by a few data structure layout changes:

c346dca10840a (YOSHIFUJI Hideaki        2008-03-25 21:47:49 +0900  430)         net = dev_net(rt->u.dst.dev);
d8d1f30b95a63 (Changli Gao              2010-06-10 23:31:35 -0700  585)         net = dev_net(rt->dst.dev);

It was then changed to fix a NULL pointer deref:

e2c693934194f (Hangbin Liu              2019-08-22 22:19:48 +0800  586) 
e2c693934194f (Hangbin Liu              2019-08-22 22:19:48 +0800  587)         if (rt->dst.dev)
e2c693934194f (Hangbin Liu              2019-08-22 22:19:48 +0800  588)                 net = dev_net(rt->dst.dev);
e2c693934194f (Hangbin Liu              2019-08-22 22:19:48 +0800  589)         else if (skb_in->dev)
e2c693934194f (Hangbin Liu              2019-08-22 22:19:48 +0800  590)                 net = dev_net(skb_in->dev);
e2c693934194f (Hangbin Liu              2019-08-22 22:19:48 +0800  591)         else
e2c693934194f (Hangbin Liu              2019-08-22 22:19:48 +0800  592)                 goto out;


> And when I read fallback in your commit message description, I
> imagined that you would have a two tiered lookup scheme.  First you
> would be trying the skb_in->dev for a lookup (to accomodate the VRF
> case), and if that failed you'd try again with skb_dst()->dev.

The code I proposed basically does use the skb_in->dev (if non-null)
for looking up which VRF table to use, else use skb_dst(skb_in) (if non-null)
for looking up which VRF table to use, else route_lookup_dev is NULL, which
means use the master table.

Whether this should instead try to lookup the source address with the skb_in->dev
table, and of that fails go to the next, is a good question. I think the context
I am missing in order to understand which approach is appropriate is which
scenario can cause skb_in->dev to be NULL, and which can cause skb_dst(skb_in)
to be NULL, and what is the expected behavior for icmp error route lookup in those
cases ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

  reply	other threads:[~2020-08-13 13:13 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20200811195003.1812-1-mathieu.desnoyers@efficios.com>
2020-08-11 19:50 ` [PATCH 1/3] selftests: Add VRF icmp error route lookup test Mathieu Desnoyers
2020-08-13 23:13   ` David Ahern
2020-08-14 14:05     ` Michael Jeanson
2020-08-11 19:50 ` [PATCH 2/3] ipv4/icmp: l3mdev: Perform icmp error route lookup on source device routing table Mathieu Desnoyers
2020-08-12 21:43   ` David Miller
2020-08-13 13:12     ` Mathieu Desnoyers [this message]
2020-08-13 22:59   ` David Ahern
2020-08-11 19:50 ` [PATCH 3/3] ipv6/icmp: " Mathieu Desnoyers
2020-08-13 23:21   ` David Ahern

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1251597699.6518.1597324378911.JavaMail.zimbra@efficios.com \
    --to=mathieu.desnoyers@efficios.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).