Re: [PATCH net] fib_semantics: Don't match route with mismatching tclassid

Re: [PATCH net] fib_semantics: Don't match route with mismatching tclassid

[David Ahern]

On 2/14/18 10:28 AM, Stefano Brivio wrote:
> In fib_nh_match(), if output interface or gateway are passed in
> the FIB configuration, we don't have to check next hops of
> multipath routes to conclude whether we have a match or not.
> 
> However, we might still have routes with different realms
> matching the same output interface and gateway configuration,
> and this needs to cause the match to fail. Otherwise the first
> route inserted in the FIB will match, regardless of the realms:
> 
>  # ip route add 1.1.1.1 dev eth0 table 1234 realms 1/2
>  # ip route append 1.1.1.1 dev eth0 table 1234 realms 3/4
>  # ip route list table 1234
>  1.1.1.1 dev eth0 scope link realms 1/2
>  1.1.1.1 dev eth0 scope link realms 3/4
>  # ip route del 1.1.1.1 dev ens3 table 1234 realms 3/4
>  # ip route list table 1234
>  1.1.1.1 dev ens3 scope link realms 3/4
> 
> whereas route with realms 3/4 should have been deleted instead.
> 
> Explicitly check for fc_flow passed in the FIB configuration
> (this comes from RTA_FLOW extracted by rtm_to_fib_config()) and
> fail matching if it differs from nh_tclassid.
> 
> The handling of RTA_FLOW for multipath routes later in
> fib_nh_match() is still needed, as we can have multiple RTA_FLOW
> attributes that need to be matched against the tclassid of each
> next hop.
> 
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  net/ipv4/fib_semantics.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index c586597da20d..2223ebac5b64 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -646,6 +646,10 @@ int fib_nh_match(struct fib_config *cfg, struct fib_info *fi,
>  					    fi->fib_nh, cfg, extack))
>  				return 1;
>  		}
> +#ifdef CONFIG_IP_ROUTE_CLASSID
> +		if (cfg->fc_flow != fi->fib_nh->nh_tclassid)
> +			return 1;
> +#endif

you should verify fc_flow is set. user is allowed to delete first
matching route based on data that is given.


>  		if ((!cfg->fc_oif || cfg->fc_oif == fi->fib_nh->nh_oif) &&
>  		    (!cfg->fc_gw  || cfg->fc_gw == fi->fib_nh->nh_gw))
>  			return 0;
> 

Re: [PATCH net] fib_semantics: Don't match route with mismatching tclassid

[Stefano Brivio]

Hi David,

On Wed, 14 Feb 2018 10:46:05 -0700
David Ahern <dsahern@gmail.com> wrote:

> On 2/14/18 10:28 AM, Stefano Brivio wrote:
>
> > +#ifdef CONFIG_IP_ROUTE_CLASSID
> > +		if (cfg->fc_flow != fi->fib_nh->nh_tclassid)
> > +			return 1;
> > +#endif  
> 
> you should verify fc_flow is set. user is allowed to delete first
> matching route based on data that is given.

Thanks for your comment.

My intended semantic was different, that is, I assumed that if fc_flow
is not set (zero, here), the user shouldn't be able to just delete the
first matching route. But I guess your interpretation makes more sense
because it's consistent with the other checks. I'll send a v2.

-- 
Stefano