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

From: David Ahern <dsahern@gmail.com>
To: Ido Schimmel <idosch@idosch.org>,
	Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
Cc: netdev@vger.kernel.org, David Miller <davem@davemloft.net>,
	Stephen Hemminger <stephen@networkplumber.org>,
	Ido Schimmel <idosch@nvidia.com>,
	Jakub Kicinski <kuba@kernel.org>, Roopa Prabhu <roopa@nvidia.com>,
	Andrei Vagin <avagin@gmail.com>,
	Pavel Tikhomirov <ptikhomirov@virtuozzo.com>,
	Alexander Mikhalitsyn <alexander@mihalicyn.com>
Subject: Re: [PATCH net-next] rtnetlink: add RTNH_REJECT_MASK
Date: Sun, 28 Nov 2021 17:19:38 -0700	[thread overview]
Message-ID: <e3d13710-2780-5dff-3cbf-fa0fd7cb5d32@gmail.com> (raw)
In-Reply-To: <YaOLt2M1hBnoVFKd@shredder>

On 11/28/21 7:01 AM, Ido Schimmel wrote:
> 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;

I like the positive naming - RTNH_VALID_USER_FLAGS.

nexthop API should allow the OFFLOAD flag to be consistent; separate
change though.