diff mbox series

[net-next,v3,4/5] net/ipv6: set expires in modify_prefix_route() if RTF_EXPIRES is set.

Message ID 20240202082200.227031-5-thinker.li@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Remove expired routes with a separated list of routes. | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1048 this patch: 1048
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1065 this patch: 1065
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1065 this patch: 1065
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-04--21-00 (tests: 721)

Commit Message

Kui-Feng Lee Feb. 2, 2024, 8:21 a.m. UTC 
From: Kui-Feng Lee <thinker.li@gmail.com>

Make the decision to set or clean the expires of a route based on the
RTF_EXPIRES flag, rather than the value of the "expires" argument.

The function inet6_addr_modify() is the only caller of
modify_prefix_route(), and it passes the RTF_EXPIRES flag and an expiration
value. The RTF_EXPIRES flag is turned on or off based on the value of
valid_lft. The RTF_EXPIRES flag is turned on if valid_lft is a finite value
(not infinite, not 0xffffffff). Even if valid_lft is 0, the RTF_EXPIRES
flag remains on. The expiration value being passed is equal to the
valid_lft value if the flag is on. However, if the valid_lft value is
infinite, the expiration value becomes 0 and the RTF_EXPIRES flag is turned
off. Despite this, modify_prefix_route() decides to set the expiration
value if the received expiration value is not zero. This mixing of infinite
and zero cases creates an inconsistency.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 net/ipv6/addrconf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Hangbin Liu Feb. 2, 2024, 12:16 p.m. UTC | #1 
On Fri, Feb 02, 2024 at 12:21:59AM -0800, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Make the decision to set or clean the expires of a route based on the
> RTF_EXPIRES flag, rather than the value of the "expires" argument.
> 
> The function inet6_addr_modify() is the only caller of
> modify_prefix_route(), and it passes the RTF_EXPIRES flag and an expiration
> value. The RTF_EXPIRES flag is turned on or off based on the value of
> valid_lft. The RTF_EXPIRES flag is turned on if valid_lft is a finite value
> (not infinite, not 0xffffffff). Even if valid_lft is 0, the RTF_EXPIRES
> flag remains on. The expiration value being passed is equal to the
> valid_lft value if the flag is on. However, if the valid_lft value is
> infinite, the expiration value becomes 0 and the RTF_EXPIRES flag is turned
> off. Despite this, modify_prefix_route() decides to set the expiration
> value if the received expiration value is not zero. This mixing of infinite
> and zero cases creates an inconsistency.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  net/ipv6/addrconf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 36bfa987c314..2f6cf6314646 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4788,7 +4788,7 @@ static int modify_prefix_route(struct inet6_ifaddr *ifp,
>  	} else {
>  		table = f6i->fib6_table;
>  		spin_lock_bh(&table->tb6_lock);
> -		if (!expires) {
> +		if (!(flags & RTF_EXPIRES)) {

Hi Kui-Feng,

I may missed something. But I still could not get why we shouldn't use
expires for checking? If expires == 0, but RTF_EXPIRES is on,
shouldn't we call fib6_clean_expires()?

Thanks
Hangbin
>  			fib6_clean_expires(f6i);
>  			fib6_remove_gc_list(f6i);
>  		} else {
> -- 
> 2.34.1
>
Kui-Feng Lee Feb. 2, 2024, 5:57 p.m. UTC | #2 
On 2/2/24 04:16, Hangbin Liu wrote:
> On Fri, Feb 02, 2024 at 12:21:59AM -0800, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Make the decision to set or clean the expires of a route based on the
>> RTF_EXPIRES flag, rather than the value of the "expires" argument.
>>
>> The function inet6_addr_modify() is the only caller of
>> modify_prefix_route(), and it passes the RTF_EXPIRES flag and an expiration
>> value. The RTF_EXPIRES flag is turned on or off based on the value of
>> valid_lft. The RTF_EXPIRES flag is turned on if valid_lft is a finite value
>> (not infinite, not 0xffffffff). Even if valid_lft is 0, the RTF_EXPIRES
>> flag remains on. The expiration value being passed is equal to the
>> valid_lft value if the flag is on. However, if the valid_lft value is
>> infinite, the expiration value becomes 0 and the RTF_EXPIRES flag is turned
>> off. Despite this, modify_prefix_route() decides to set the expiration
>> value if the received expiration value is not zero. This mixing of infinite
>> and zero cases creates an inconsistency.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   net/ipv6/addrconf.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index 36bfa987c314..2f6cf6314646 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -4788,7 +4788,7 @@ static int modify_prefix_route(struct inet6_ifaddr *ifp,
>>   	} else {
>>   		table = f6i->fib6_table;
>>   		spin_lock_bh(&table->tb6_lock);
>> -		if (!expires) {
>> +		if (!(flags & RTF_EXPIRES)) {
> 
> Hi Kui-Feng,
> 
> I may missed something. But I still could not get why we shouldn't use
> expires for checking? If expires == 0, but RTF_EXPIRES is on,
> shouldn't we call fib6_clean_expires()?


The case that expires == 0 and RTF_EXPIES is on never happens since
inet6_addr_modify() rejects valid_lft == 0 at the beginning. This
patch doesn't make difference logically, but make inet6_addr_modify()
and modify_prefix_route() consistent.

Does that make sense to you?


> 
> Thanks
> Hangbin
>>   			fib6_clean_expires(f6i);
>>   			fib6_remove_gc_list(f6i);
>>   		} else {
>> -- 
>> 2.34.1
>>
Hangbin Liu Feb. 4, 2024, 10:17 a.m. UTC | #3 
On Fri, Feb 02, 2024 at 09:57:46AM -0800, Kui-Feng Lee wrote:
> > Hi Kui-Feng,
> > 
> > I may missed something. But I still could not get why we shouldn't use
> > expires for checking? If expires == 0, but RTF_EXPIRES is on,
> > shouldn't we call fib6_clean_expires()?
> 
> 
> The case that expires == 0 and RTF_EXPIES is on never happens since
> inet6_addr_modify() rejects valid_lft == 0 at the beginning. This
> patch doesn't make difference logically, but make inet6_addr_modify()
> and modify_prefix_route() consistent.
> 
> Does that make sense to you?

Thanks, this does make sense to me. If there will be a new version. It would
be good to add the following sentence in the description.

"""
This patch doesn't make difference logically, but make inet6_addr_modify()
and modify_prefix_route() consistent.
"""

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>

Regards
Hangbin
Kui-Feng Lee Feb. 5, 2024, 6:59 p.m. UTC | #4 
On 2/4/24 02:17, Hangbin Liu wrote:
> On Fri, Feb 02, 2024 at 09:57:46AM -0800, Kui-Feng Lee wrote:
>>> Hi Kui-Feng,
>>>
>>> I may missed something. But I still could not get why we shouldn't use
>>> expires for checking? If expires == 0, but RTF_EXPIRES is on,
>>> shouldn't we call fib6_clean_expires()?
>>
>>
>> The case that expires == 0 and RTF_EXPIES is on never happens since
>> inet6_addr_modify() rejects valid_lft == 0 at the beginning. This
>> patch doesn't make difference logically, but make inet6_addr_modify()
>> and modify_prefix_route() consistent.
>>
>> Does that make sense to you?
> 
> Thanks, this does make sense to me. If there will be a new version. It would
> be good to add the following sentence in the description.
> 
> """
> This patch doesn't make difference logically, but make inet6_addr_modify()
> and modify_prefix_route() consistent.
> """
> 

Sure, I will add it to the commit message.


> Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> Regards
> Hangbin
diff mbox series

Patch

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 36bfa987c314..2f6cf6314646 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4788,7 +4788,7 @@  static int modify_prefix_route(struct inet6_ifaddr *ifp,
 	} else {
 		table = f6i->fib6_table;
 		spin_lock_bh(&table->tb6_lock);
-		if (!expires) {
+		if (!(flags & RTF_EXPIRES)) {
 			fib6_clean_expires(f6i);
 			fib6_remove_gc_list(f6i);
 		} else {