From: "liujian (CE)" <liujian56@huawei.com>
To: Julian Anastasov <ja@ssi.bg>
Cc: <davem@davemloft.net>, <dsahern@kernel.org>,
<edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
<hadi@cyberus.ca>, <netdev@vger.kernel.org>
Subject: Re: [PATCH net] net: ipv4: fix one memleak in __inet_del_ifa()
Date: Wed, 6 Sep 2023 11:39:03 +0800 [thread overview]
Message-ID: <cbe579bd-4aff-8239-0e32-6acb79b11bca@huawei.com> (raw)
In-Reply-To: <bcb0e791-37ab-3fff-9da6-a86883924205@ssi.bg>
On 2023/9/6 1:20, Julian Anastasov wrote:
>
> Hello,
>
> 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.
>>
>> 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
> 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>
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 5deac0517ef7..7c71fa8996bb 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -355,14 +355,12 @@ 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 *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);
> if (in_dev->dead)
> goto no_promotions;
>
> @@ -374,10 +372,6 @@ static void __inet_del_ifa(struct in_device *in_dev,
> struct in_ifaddr __rcu **ifap1 = &ifa1->ifa_next;
>
> while ((ifa = rtnl_dereference(*ifap1)) != NULL) {
> - if (!(ifa->ifa_flags & IFA_F_SECONDARY) &&
> - ifa1->ifa_scope <= ifa->ifa_scope)
> - last_prim = ifa;
> -
> if (!(ifa->ifa_flags & IFA_F_SECONDARY) ||
> ifa1->ifa_mask != ifa->ifa_mask ||
> !inet_ifa_match(ifa1->ifa_address, ifa)) {
> @@ -415,7 +409,7 @@ static void __inet_del_ifa(struct in_device *in_dev,
> no_promotions:
> /* 2. Unlink it */
>
> - *ifap = ifa1->ifa_next;
> + rcu_assign_pointer(*ifap, rtnl_dereference(ifa1->ifa_next));
> inet_hash_remove(ifa1);
>
> /* 3. Announce address deletion */
> @@ -440,9 +434,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(*ifap);
> rcu_assign_pointer(promote->ifa_next, last_sec);
> - rcu_assign_pointer(last_prim->ifa_next, promote);
> + rcu_assign_pointer(*ifap, promote);
> }
>
> promote->ifa_flags &= ~IFA_F_SECONDARY;
>>
>> Fixes: 0ff60a45678e ("[IPV4]: Fix secondary IP addresses after promotion")
>> Signed-off-by: Liu Jian <liujian56@huawei.com>
>> ---
>> net/ipv4/devinet.c | 26 ++++++++++++++++++++------
>> 1 file changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
>> index 9cf64ee47dd2..99278f4b58e0 100644
>> --- a/net/ipv4/devinet.c
>> +++ b/net/ipv4/devinet.c
>> @@ -355,14 +355,13 @@ 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 *last_prim = NULL;
>> 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);
>> if (in_dev->dead)
>> goto no_promotions;
>>
>> @@ -371,7 +370,16 @@ static void __inet_del_ifa(struct in_device *in_dev,
>> **/
>>
>> if (!(ifa1->ifa_flags & IFA_F_SECONDARY)) {
>> - struct in_ifaddr __rcu **ifap1 = &ifa1->ifa_next;
>> + struct in_ifaddr __rcu **ifap1 = &in_dev->ifa_list;
>> +
>> + while ((ifa = rtnl_dereference(*ifap1)) != NULL) {
>> + if (ifa1 == ifa)
>> + break;
>> + last_prim = ifa;
>> + ifap1 = &ifa->ifa_next;
>> + }
>> +
>> + ifap1 = &ifa1->ifa_next;
>>
>> while ((ifa = rtnl_dereference(*ifap1)) != NULL) {
>> if (!(ifa->ifa_flags & IFA_F_SECONDARY) &&
>> @@ -440,9 +448,15 @@ 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);
>> - rcu_assign_pointer(promote->ifa_next, last_sec);
>> - rcu_assign_pointer(last_prim->ifa_next, promote);
>> + if (last_prim) {
>> + last_sec = rtnl_dereference(last_prim->ifa_next);
>> + rcu_assign_pointer(promote->ifa_next, last_sec);
>> + rcu_assign_pointer(last_prim->ifa_next, promote);
>> + } else {
>> + rcu_assign_pointer(promote->ifa_next,
>> + rtnl_dereference(in_dev->ifa_list));
>> + rcu_assign_pointer(in_dev->ifa_list, promote);
>> + }
>> }
>>
>> promote->ifa_flags &= ~IFA_F_SECONDARY;
>> --
>> 2.34.1
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
>
next prev parent reply other threads:[~2023-09-06 3:39 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-05 13:55 [PATCH net] net: ipv4: fix one memleak in __inet_del_ifa() Liu Jian
2023-09-05 15:57 ` Zhu Yanjun
2023-09-05 17:20 ` Julian Anastasov
2023-09-06 3:39 ` liujian (CE) [this message]
2023-09-06 10:30 ` Julian Anastasov
2023-09-07 2:54 ` liujian (CE)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cbe579bd-4aff-8239-0e32-6acb79b11bca@huawei.com \
--to=liujian56@huawei.com \
--cc=davem@davemloft.net \
--cc=dsahern@kernel.org \
--cc=edumazet@google.com \
--cc=hadi@cyberus.ca \
--cc=ja@ssi.bg \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).