Re: [PATCH net-next] rtnetlink: add RTNH_REJECT_MASK

[Ido Schimmel <idosch@idosch.org>]

On Fri, Nov 26, 2021 at 04:43:11PM +0300, Alexander Mikhalitsyn wrote:
> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
> index 5888492a5257..9c065e2fdef9 100644
> --- a/include/uapi/linux/rtnetlink.h
> +++ b/include/uapi/linux/rtnetlink.h
> @@ -417,6 +417,9 @@ struct rtnexthop {
>  #define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | \
>  				 RTNH_F_OFFLOAD | RTNH_F_TRAP)
>  
> +/* these flags can't be set by the userspace */
> +#define RTNH_REJECT_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN)
> +
>  /* Macros to handle hexthops */
>  
>  #define RTNH_ALIGNTO	4
> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index 4c0c33e4710d..805f5e05b56d 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -685,7 +685,7 @@ static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh,
>  			return -EINVAL;
>  		}
>  
> -		if (rtnh->rtnh_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
> +		if (rtnh->rtnh_flags & RTNH_REJECT_MASK) {
>  			NL_SET_ERR_MSG(extack,
>  				       "Invalid flags for nexthop - can not contain DEAD or LINKDOWN");
>  			return -EINVAL;
> @@ -1363,7 +1363,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
>  		goto err_inval;
>  	}
>  
> -	if (cfg->fc_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
> +	if (cfg->fc_flags & RTNH_REJECT_MASK) {
>  		NL_SET_ERR_MSG(extack,
>  			       "Invalid rtm_flags - can not contain DEAD or LINKDOWN");

Instead of a deny list as in the legacy nexthop code, the new nexthop
code has an allow list (from rtm_to_nh_config()):

```
	if (nhm->nh_flags & ~NEXTHOP_VALID_USER_FLAGS) {
		NL_SET_ERR_MSG(extack, "Invalid nexthop flags in ancillary header");
		goto out;
	}
```

Where:

```
#define NEXTHOP_VALID_USER_FLAGS RTNH_F_ONLINK
```

So while the legacy nexthop code allows setting flags such as
RTNH_F_OFFLOAD, the new nexthop code denies them. I don't have a use
case for setting these flags from user space so I don't care if we allow
or deny them, but I believe the legacy and new nexthop code should be
consistent.

WDYT? Should we allow these flags in the new nexthop code as well or
keep denying them?

>  		goto err_inval;
> -- 
> 2.31.1
>