Re: [PATCH net] net: ipv4: fix one memleak in __inet_del_ifa()

["liujian (CE)" <liujian56@huawei.com>]

On 2023/9/6 18:30, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Wed, 6 Sep 2023, liujian (CE) wrote:
> 
>> On 2023/9/6 1:20, Julian Anastasov wrote:
>>>
>>> On Tue, 5 Sep 2023, Liu Jian wrote:
>>>
>>>> I got the below warning when do fuzzing test:
>>>> unregister_netdevice: waiting for bond0 to become free. Usage count = 2
>>>>
>>>> It can be repoduced via:
>>>>
>>>> ip link add bond0 type bond
>>>> sysctl -w net.ipv4.conf.bond0.promote_secondaries=1
>>>> ip addr add 4.117.174.103/0 scope 0x40 dev bond0
>>>> ip addr add 192.168.100.111/255.255.255.254 scope 0 dev bond0
>>>> ip addr add 0.0.0.4/0 scope 0x40 secondary dev bond0
>>>> ip addr del 4.117.174.103/0 scope 0x40 dev bond0
>>>> ip link delete bond0 type bond
>>>>
>>>> In this reproduction test case, an incorrect 'last_prim' is found in
>>>> __inet_del_ifa(), as a result, the secondary address(0.0.0.4/0 scope 0x40)
>>>> is lost. The memory of the secondary address is leaked and the reference of
>>>> in_device and net_device is leaked.
> 
> 	We can also explain that the problem occurs when we delete
> the first primary address and the promoted address is leaked because
> it is attached to the to-be-freed primary address instead of to ifa_list.
> 
>>>> Fix this problem by modifying the PROMOTE_SECONDANCE behavior as follows:
>>>> 1. Traverse in_dev->ifa_list to search for the actual 'last_prim'.
>>>> 2. When last_prim is empty, move 'promote' to the in_dev->ifa_list header.
>>>
>>> 	So, the problem is that last_prim initially points to the
>>> first primary address that we are actually removing. Looks like with
>>> last_prim we try to promote the secondary IP after all primaries with
>>> scope >= our scope, i.e. simulating a new IP insert. As the secondary IPs
>>> have same scope as their primary, why just not remove the last_prim
>>> var/code and to insert the promoted secondary at the same place as the
>>> deleted primary? May be your patch does the same: insert at same pos?
>>>
>>> Before deletion:
>>> 1. primary1 scope global (to be deleted)
>>> 2. primary2 scope global
>>> 3. promoted_secondary
>>>
>>> After deletion (old way, promote as a new insertion):
>>> 1. primary2 scope global
>>> 2. promoted_secondary scope global (inserted as new primary)
>>>
>> It is :
>> After deletion (old way, promoted_secondary lost):
>> 1. primary2 scope global
> 
> 	Yes, that is what happens :)
> 
>>> After deletion (new way, promote at same place):
>>> 1. promoted_secondary scope global (now primary, inserted at same place)
>>> 2. primary2 scope global
>>>
>>>   What I mean is to use ifap as last_prim, not tested:
>>>
>> Yes, This is better and it can work also. Thanks.
>> Tested-by: Liu Jian <liujian56@huawei.com>
> 
> 	But let me propose another version. It is a minimal
> bugfix that does not change the place where the promoted address
> is added and just converts last_prim to be rcu ptr to insert
> position. last_prim will start from ifap because the promoted
> address can not be added before this position. It is better
> not to reorder the IPs because scripts may depend on the
> old behavior to add the promoted IP after all others for
> the scope. If you find this version better you can post it
> as v2 and I'll sign it too.
Have sent v2, thank you for this case.
> 
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 5deac0517ef7..37be82496322 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -355,14 +355,14 @@ static void __inet_del_ifa(struct in_device *in_dev,
>   {
>   	struct in_ifaddr *promote = NULL;
>   	struct in_ifaddr *ifa, *ifa1;
> -	struct in_ifaddr *last_prim;
> +	struct in_ifaddr __rcu **last_prim;
>   	struct in_ifaddr *prev_prom = NULL;
>   	int do_promote = IN_DEV_PROMOTE_SECONDARIES(in_dev);
>   
>   	ASSERT_RTNL();
>   
>   	ifa1 = rtnl_dereference(*ifap);
> -	last_prim = rtnl_dereference(in_dev->ifa_list);
> +	last_prim = ifap;
>   	if (in_dev->dead)
>   		goto no_promotions;
>   
> @@ -376,7 +376,7 @@ static void __inet_del_ifa(struct in_device *in_dev,
>   		while ((ifa = rtnl_dereference(*ifap1)) != NULL) {
>   			if (!(ifa->ifa_flags & IFA_F_SECONDARY) &&
>   			    ifa1->ifa_scope <= ifa->ifa_scope)
> -				last_prim = ifa;
> +				last_prim = &ifa->ifa_next;
>   
>   			if (!(ifa->ifa_flags & IFA_F_SECONDARY) ||
>   			    ifa1->ifa_mask != ifa->ifa_mask ||
> @@ -440,9 +440,9 @@ static void __inet_del_ifa(struct in_device *in_dev,
>   
>   			rcu_assign_pointer(prev_prom->ifa_next, next_sec);
>   
> -			last_sec = rtnl_dereference(last_prim->ifa_next);
> +			last_sec = rtnl_dereference(*last_prim);
>   			rcu_assign_pointer(promote->ifa_next, last_sec);
> -			rcu_assign_pointer(last_prim->ifa_next, promote);
> +			rcu_assign_pointer(*last_prim, promote);
>   		}
>   
>   		promote->ifa_flags &= ~IFA_F_SECONDARY;
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>
>