[PATCH] net: fib_rules: add FRA_GOTO and FRA_L3MDEV to fib_rule_nlmsg_size()

[PATCH] net: fib_rules: add FRA_GOTO and FRA_L3MDEV to fib_rule_nlmsg_size()

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

fib_nl_fill_rule() emits FRA_GOTO and FRA_L3MDEV, but
fib_rule_nlmsg_size() reserves room for neither. Its only caller,
notify_rule_change(), allocates the notification skb with that size and
treats a failure as a bug:

	/* -EMSGSIZE implies BUG in fib_rule_nlmsg_size() */

A rule that has both a goto target and l3mdev set needs 16 more bytes
than are reserved. nlmsg_new() rounds the allocation up through
SKB_DATA_ALIGN(), so the slack hides the mistake most of the time and
the bug stays latent, but the accounting is wrong and the notification
can still fail with -EMSGSIZE.

Reserve the two missing attributes, keeping the order used by
fib_nl_fill_rule().

Fixes: 0947c9fe56d9 ("[NET] fib_rules: goto rule action")
Fixes: 96c63fa7393d ("net: Add l3mdev rule")
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 net/core/fib_rules.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
index 7df216c17c67..5e84bc740191 100644
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c
@@ -1133,7 +1133,9 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
 			 + nla_total_size(4) /* FRA_SUPPRESS_IFGROUP */
 			 + nla_total_size(4) /* FRA_FWMARK */
 			 + nla_total_size(4) /* FRA_FWMASK */
+			 + nla_total_size(4) /* FRA_GOTO */
 			 + nla_total_size_64bit(8) /* FRA_TUN_ID */
+			 + nla_total_size(1) /* FRA_L3MDEV */
 			 + nla_total_size(sizeof(struct fib_kuid_range))
 			 + nla_total_size(1) /* FRA_PROTOCOL */
 			 + nla_total_size(1) /* FRA_IP_PROTO */
-- 
2.25.1

Re: [PATCH] net: fib_rules: add FRA_GOTO and FRA_L3MDEV to fib_rule_nlmsg_size()

[David Ahern]

On 9/11/26 2:12 AM, Linkui Xiao wrote:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> fib_nl_fill_rule() emits FRA_GOTO and FRA_L3MDEV, but
> fib_rule_nlmsg_size() reserves room for neither. Its only caller,
> notify_rule_change(), allocates the notification skb with that size and
> treats a failure as a bug:
> 
> 	/* -EMSGSIZE implies BUG in fib_rule_nlmsg_size() */

not really going to fail since the size has so many attributes that are
not applicable to this rule.

> 
> A rule that has both a goto target and l3mdev set needs 16 more bytes
> than are reserved. nlmsg_new() rounds the allocation up through
> SKB_DATA_ALIGN(), so the slack hides the mistake most of the time and
> the bug stays latent, but the accounting is wrong and the notification
> can still fail with -EMSGSIZE.
> 
> Reserve the two missing attributes, keeping the order used by
> fib_nl_fill_rule().
> 
> Fixes: 0947c9fe56d9 ("[NET] fib_rules: goto rule action")
> Fixes: 96c63fa7393d ("net: Add l3mdev rule")
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
>  net/core/fib_rules.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
> index 7df216c17c67..5e84bc740191 100644
> --- a/net/core/fib_rules.c
> +++ b/net/core/fib_rules.c
> @@ -1133,7 +1133,9 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
>  			 + nla_total_size(4) /* FRA_SUPPRESS_IFGROUP */
>  			 + nla_total_size(4) /* FRA_FWMARK */
>  			 + nla_total_size(4) /* FRA_FWMASK */
> +			 + nla_total_size(4) /* FRA_GOTO */
>  			 + nla_total_size_64bit(8) /* FRA_TUN_ID */
> +			 + nla_total_size(1) /* FRA_L3MDEV */
>  			 + nla_total_size(sizeof(struct fib_kuid_range))
>  			 + nla_total_size(1) /* FRA_PROTOCOL */
>  			 + nla_total_size(1) /* FRA_IP_PROTO */

Re: [PATCH] net: fib_rules: add FRA_GOTO and FRA_L3MDEV to fib_rule_nlmsg_size()

[Linkui Xiao]

On 2026/9/13 09:06, David Ahern wrote:
> On 9/11/26 2:12 AM, Linkui Xiao wrote:
>> From: Linkui Xiao <xiaolinkui@kylinos.cn>
>>
>> fib_nl_fill_rule() emits FRA_GOTO and FRA_L3MDEV, but
>> fib_rule_nlmsg_size() reserves room for neither. Its only caller,
>> notify_rule_change(), allocates the notification skb with that size and
>> treats a failure as a bug:
>>
>> 	/* -EMSGSIZE implies BUG in fib_rule_nlmsg_size() */
> not really going to fail since the size has so many attributes that are
> not applicable to this rule.
You are right, the other attributes reserved in fib_rule_nlmsg_size()
already provide more than enough slack for the common case, so the
missing FRA_GOTO/FRA_L3MDEV accounting cannot actually overflow the skb.

I will drop this patch. Thanks for taking the time to review it.

Thanks,
Linkui Xiao

>
>> A rule that has both a goto target and l3mdev set needs 16 more bytes
>> than are reserved. nlmsg_new() rounds the allocation up through
>> SKB_DATA_ALIGN(), so the slack hides the mistake most of the time and
>> the bug stays latent, but the accounting is wrong and the notification
>> can still fail with -EMSGSIZE.
>>
>> Reserve the two missing attributes, keeping the order used by
>> fib_nl_fill_rule().
>>
>> Fixes: 0947c9fe56d9 ("[NET] fib_rules: goto rule action")
>> Fixes: 96c63fa7393d ("net: Add l3mdev rule")
>> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
>> ---
>>   net/core/fib_rules.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
>> index 7df216c17c67..5e84bc740191 100644
>> --- a/net/core/fib_rules.c
>> +++ b/net/core/fib_rules.c
>> @@ -1133,7 +1133,9 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
>>   			 + nla_total_size(4) /* FRA_SUPPRESS_IFGROUP */
>>   			 + nla_total_size(4) /* FRA_FWMARK */
>>   			 + nla_total_size(4) /* FRA_FWMASK */
>> +			 + nla_total_size(4) /* FRA_GOTO */
>>   			 + nla_total_size_64bit(8) /* FRA_TUN_ID */
>> +			 + nla_total_size(1) /* FRA_L3MDEV */
>>   			 + nla_total_size(sizeof(struct fib_kuid_range))
>>   			 + nla_total_size(1) /* FRA_PROTOCOL */
>>   			 + nla_total_size(1) /* FRA_IP_PROTO */

Re: [PATCH] net: fib_rules: add FRA_GOTO and FRA_L3MDEV to fib_rule_nlmsg_size()

[Linkui Xiao]

On 2026/9/13 09:06, David Ahern wrote:
> On 9/11/26 2:12 AM, Linkui Xiao wrote:
>> From: Linkui Xiao <xiaolinkui@kylinos.cn>
>>
>> fib_nl_fill_rule() emits FRA_GOTO and FRA_L3MDEV, but
>> fib_rule_nlmsg_size() reserves room for neither. Its only caller,
>> notify_rule_change(), allocates the notification skb with that size and
>> treats a failure as a bug:
>>
>> 	/* -EMSGSIZE implies BUG in fib_rule_nlmsg_size() */
> 
> not really going to fail since the size has so many attributes that are
> not applicable to this rule.
> 
You are right, the other attributes reserved in fib_rule_nlmsg_size()
already provide more than enough slack for the common case, so the
missing FRA_GOTO/FRA_L3MDEV accounting cannot actually overflow the skb.

I will drop this patch. Thanks for taking the time to review it.

Thanks,
Linkui Xiao
>>
>> A rule that has both a goto target and l3mdev set needs 16 more bytes
>> than are reserved. nlmsg_new() rounds the allocation up through
>> SKB_DATA_ALIGN(), so the slack hides the mistake most of the time and
>> the bug stays latent, but the accounting is wrong and the notification
>> can still fail with -EMSGSIZE.
>>
>> Reserve the two missing attributes, keeping the order used by
>> fib_nl_fill_rule().
>>
>> Fixes: 0947c9fe56d9 ("[NET] fib_rules: goto rule action")
>> Fixes: 96c63fa7393d ("net: Add l3mdev rule")
>> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
>> ---
>>   net/core/fib_rules.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
>> index 7df216c17c67..5e84bc740191 100644
>> --- a/net/core/fib_rules.c
>> +++ b/net/core/fib_rules.c
>> @@ -1133,7 +1133,9 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
>>   			 + nla_total_size(4) /* FRA_SUPPRESS_IFGROUP */
>>   			 + nla_total_size(4) /* FRA_FWMARK */
>>   			 + nla_total_size(4) /* FRA_FWMASK */
>> +			 + nla_total_size(4) /* FRA_GOTO */
>>   			 + nla_total_size_64bit(8) /* FRA_TUN_ID */
>> +			 + nla_total_size(1) /* FRA_L3MDEV */
>>   			 + nla_total_size(sizeof(struct fib_kuid_range))
>>   			 + nla_total_size(1) /* FRA_PROTOCOL */
>>   			 + nla_total_size(1) /* FRA_IP_PROTO */
>