From: Hangbin Liu <[email protected]>
[ Upstream commit e9919a24d3022f72bcadc407e73a6ef17093a849 ]
With commit 153380ec4b9 ("fib_rules: Added NLM_F_EXCL support to
fib_nl_newrule") we now able to check if a rule already exists. But this
only works with iproute2. For other tools like libnl, NetworkManager,
it still could add duplicate rules with only NLM_F_CREATE flag, like
[localhost ~ ]# ip rule
0: from all lookup local
32766: from all lookup main
32767: from all lookup default
100000: from 192.168.7.5 lookup 5
100000: from 192.168.7.5 lookup 5
As it doesn't make sense to create two duplicate rules, let's just return
0 if the rule exists.
Fixes: 153380ec4b9 ("fib_rules: Added NLM_F_EXCL support to fib_nl_newrule")
Reported-by: Thomas Haller <[email protected]>
Signed-off-by: Hangbin Liu <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
net/core/fib_rules.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c
@@ -429,9 +429,9 @@ int fib_nl_newrule(struct sk_buff *skb,
if (rule->l3mdev && rule->table)
goto errout_free;
- if ((nlh->nlmsg_flags & NLM_F_EXCL) &&
- rule_exists(ops, frh, tb, rule)) {
- err = -EEXIST;
+ if (rule_exists(ops, frh, tb, rule)) {
+ if (nlh->nlmsg_flags & NLM_F_EXCL)
+ err = -EEXIST;
goto errout_free;
}
On Wed, May 15, 2019 at 12:56:16PM +0200, Greg Kroah-Hartman wrote:
> From: Hangbin Liu <[email protected]>
>
> [ Upstream commit e9919a24d3022f72bcadc407e73a6ef17093a849 ]
>
> With commit 153380ec4b9 ("fib_rules: Added NLM_F_EXCL support to
> fib_nl_newrule") we now able to check if a rule already exists. But this
> only works with iproute2. For other tools like libnl, NetworkManager,
> it still could add duplicate rules with only NLM_F_CREATE flag, like
>
> [localhost ~ ]# ip rule
> 0: from all lookup local
> 32766: from all lookup main
> 32767: from all lookup default
> 100000: from 192.168.7.5 lookup 5
> 100000: from 192.168.7.5 lookup 5
>
> As it doesn't make sense to create two duplicate rules, let's just return
> 0 if the rule exists.
>
> Fixes: 153380ec4b9 ("fib_rules: Added NLM_F_EXCL support to fib_nl_newrule")
> Reported-by: Thomas Haller <[email protected]>
> Signed-off-by: Hangbin Liu <[email protected]>
> Signed-off-by: David S. Miller <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> net/core/fib_rules.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> --- a/net/core/fib_rules.c
> +++ b/net/core/fib_rules.c
> @@ -429,9 +429,9 @@ int fib_nl_newrule(struct sk_buff *skb,
> if (rule->l3mdev && rule->table)
> goto errout_free;
>
> - if ((nlh->nlmsg_flags & NLM_F_EXCL) &&
> - rule_exists(ops, frh, tb, rule)) {
> - err = -EEXIST;
> + if (rule_exists(ops, frh, tb, rule)) {
> + if (nlh->nlmsg_flags & NLM_F_EXCL)
> + err = -EEXIST;
> goto errout_free;
> }
>
>
>
Hi all,
This commit is causing issues on Android devices when Wi-Fi and mobile
data are both enabled. The device will do a soft reboot consistently.
So far, I've had reports on the Pixel 3 XL, OnePlus 6, Pocophone, and
Note 9 and I can reproduce on my OnePlus 6.
Sorry for taking so long to report this, I just figured out how to
reproduce it today and I didn't want to report it without that.
Attached is a full dmesg and the relevant snippet from Android's logcat.
Let me know what I can do to help debug,
Nathan
Nathan Chancellor <[email protected]> wrote:
> On Wed, May 15, 2019 at 12:56:16PM +0200, Greg Kroah-Hartman wrote:
> > From: Hangbin Liu <[email protected]>
> >
> > [ Upstream commit e9919a24d3022f72bcadc407e73a6ef17093a849 ]
[..]
> > Fixes: 153380ec4b9 ("fib_rules: Added NLM_F_EXCL support to fib_nl_newrule")
> > Reported-by: Thomas Haller <[email protected]>
> > Signed-off-by: Hangbin Liu <[email protected]>
> > Signed-off-by: David S. Miller <[email protected]>
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > ---
> > net/core/fib_rules.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > --- a/net/core/fib_rules.c
> > +++ b/net/core/fib_rules.c
> > @@ -429,9 +429,9 @@ int fib_nl_newrule(struct sk_buff *skb,
> > if (rule->l3mdev && rule->table)
> > goto errout_free;
> >
> > - if ((nlh->nlmsg_flags & NLM_F_EXCL) &&
> > - rule_exists(ops, frh, tb, rule)) {
> > - err = -EEXIST;
> > + if (rule_exists(ops, frh, tb, rule)) {
> > + if (nlh->nlmsg_flags & NLM_F_EXCL)
> > + err = -EEXIST;
> This commit is causing issues on Android devices when Wi-Fi and mobile
> data are both enabled. The device will do a soft reboot consistently.
Not surprising, the patch can't be applied to 4.9 as-is.
In 4.9, code looks like this:
err = -EINVAL;
/* irrelevant */
if (rule_exists(ops, frh, tb, rule)) {
if (nlh->nlmsg_flags & NLM_F_EXCL)
err = -EEXIST;
goto errout_free;
}
So, if rule_exists() is true, we return -EINVAL to caller
instead of 0, unlike upstream.
I don't think this commit is stable material.
On 5/19/19 9:43 AM, Nathan Chancellor wrote:
> Hi all,
>
> This commit is causing issues on Android devices when Wi-Fi and mobile
> data are both enabled. The device will do a soft reboot consistently.
> So far, I've had reports on the Pixel 3 XL, OnePlus 6, Pocophone, and
> Note 9 and I can reproduce on my OnePlus 6.
>
> Sorry for taking so long to report this, I just figured out how to
> reproduce it today and I didn't want to report it without that.
>
> Attached is a full dmesg and the relevant snippet from Android's logcat.
>
> Let me know what I can do to help debug,
> Nathan
>
It's a backport problem. err needs to be reset to 0 before the goto.
On Sun, May 19, 2019 at 08:43:48AM -0700, Nathan Chancellor wrote:
>On Wed, May 15, 2019 at 12:56:16PM +0200, Greg Kroah-Hartman wrote:
>> From: Hangbin Liu <[email protected]>
>>
>> [ Upstream commit e9919a24d3022f72bcadc407e73a6ef17093a849 ]
>>
>> With commit 153380ec4b9 ("fib_rules: Added NLM_F_EXCL support to
>> fib_nl_newrule") we now able to check if a rule already exists. But this
>> only works with iproute2. For other tools like libnl, NetworkManager,
>> it still could add duplicate rules with only NLM_F_CREATE flag, like
>>
>> [localhost ~ ]# ip rule
>> 0: from all lookup local
>> 32766: from all lookup main
>> 32767: from all lookup default
>> 100000: from 192.168.7.5 lookup 5
>> 100000: from 192.168.7.5 lookup 5
>>
>> As it doesn't make sense to create two duplicate rules, let's just return
>> 0 if the rule exists.
>>
>> Fixes: 153380ec4b9 ("fib_rules: Added NLM_F_EXCL support to fib_nl_newrule")
>> Reported-by: Thomas Haller <[email protected]>
>> Signed-off-by: Hangbin Liu <[email protected]>
>> Signed-off-by: David S. Miller <[email protected]>
>> Signed-off-by: Greg Kroah-Hartman <[email protected]>
>> ---
>> net/core/fib_rules.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> --- a/net/core/fib_rules.c
>> +++ b/net/core/fib_rules.c
>> @@ -429,9 +429,9 @@ int fib_nl_newrule(struct sk_buff *skb,
>> if (rule->l3mdev && rule->table)
>> goto errout_free;
>>
>> - if ((nlh->nlmsg_flags & NLM_F_EXCL) &&
>> - rule_exists(ops, frh, tb, rule)) {
>> - err = -EEXIST;
>> + if (rule_exists(ops, frh, tb, rule)) {
>> + if (nlh->nlmsg_flags & NLM_F_EXCL)
>> + err = -EEXIST;
>> goto errout_free;
>> }
>>
>>
>>
>
>Hi all,
>
>This commit is causing issues on Android devices when Wi-Fi and mobile
>data are both enabled. The device will do a soft reboot consistently.
>So far, I've had reports on the Pixel 3 XL, OnePlus 6, Pocophone, and
>Note 9 and I can reproduce on my OnePlus 6.
Is this something that happens with Linus's tree as well? or is this a
backport issue?
>Sorry for taking so long to report this, I just figured out how to
>reproduce it today and I didn't want to report it without that.
FWIW, if you see anything suspicious with -stable patches just let us
know separately from a "better" bug report for upstream, then we can at
least temporary pull it out of the stable queue while the issue is being
addressed.
--
Thanks,
Sasha
On Sun, May 19, 2019 at 10:27:53PM +0200, Florian Westphal wrote:
> Nathan Chancellor <[email protected]> wrote:
> > On Wed, May 15, 2019 at 12:56:16PM +0200, Greg Kroah-Hartman wrote:
> > > From: Hangbin Liu <[email protected]>
> > >
> > > [ Upstream commit e9919a24d3022f72bcadc407e73a6ef17093a849 ]
>
> [..]
>
> > > Fixes: 153380ec4b9 ("fib_rules: Added NLM_F_EXCL support to fib_nl_newrule")
> > > Reported-by: Thomas Haller <[email protected]>
> > > Signed-off-by: Hangbin Liu <[email protected]>
> > > Signed-off-by: David S. Miller <[email protected]>
> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > > ---
> > > net/core/fib_rules.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > --- a/net/core/fib_rules.c
> > > +++ b/net/core/fib_rules.c
> > > @@ -429,9 +429,9 @@ int fib_nl_newrule(struct sk_buff *skb,
> > > if (rule->l3mdev && rule->table)
> > > goto errout_free;
> > >
> > > - if ((nlh->nlmsg_flags & NLM_F_EXCL) &&
> > > - rule_exists(ops, frh, tb, rule)) {
> > > - err = -EEXIST;
> > > + if (rule_exists(ops, frh, tb, rule)) {
> > > + if (nlh->nlmsg_flags & NLM_F_EXCL)
> > > + err = -EEXIST;
> > This commit is causing issues on Android devices when Wi-Fi and mobile
> > data are both enabled. The device will do a soft reboot consistently.
>
> Not surprising, the patch can't be applied to 4.9 as-is.
>
> In 4.9, code looks like this:
>
> err = -EINVAL;
> /* irrelevant */
> if (rule_exists(ops, frh, tb, rule)) {
> if (nlh->nlmsg_flags & NLM_F_EXCL)
> err = -EEXIST;
> goto errout_free;
> }
>
> So, if rule_exists() is true, we return -EINVAL to caller
> instead of 0, unlike upstream.
>
> I don't think this commit is stable material.
Thanks Florian for helping check it. So we need either revert this patch,
or at least backport adeb45cbb505 ("fib_rules: fix error return code") and
f9d4b0c1e969 ("fib_rules: move common handling of newrule delrule
msgs into fib_nl2rule").
For me, I agree to revert this patch from stable tree as it's a small fix. The
issue has been there for a long time and I didn't see much complain from
customer.
Thanks
Hangbin
On Sun, May 19, 2019 at 06:29:19PM -0600, David Ahern wrote:
> On 5/19/19 9:43 AM, Nathan Chancellor wrote:
> > Hi all,
> >
> > This commit is causing issues on Android devices when Wi-Fi and mobile
> > data are both enabled. The device will do a soft reboot consistently.
> > So far, I've had reports on the Pixel 3 XL, OnePlus 6, Pocophone, and
> > Note 9 and I can reproduce on my OnePlus 6.
> >
> > Sorry for taking so long to report this, I just figured out how to
> > reproduce it today and I didn't want to report it without that.
> >
> > Attached is a full dmesg and the relevant snippet from Android's logcat.
> >
> > Let me know what I can do to help debug,
> > Nathan
> >
>
> It's a backport problem. err needs to be reset to 0 before the goto.
Ah, I see it, let me go queue up a fix for this.
thanks,
greg k-h
On Mon, May 20, 2019 at 11:04:29AM +0200, Greg Kroah-Hartman wrote:
> On Sun, May 19, 2019 at 06:29:19PM -0600, David Ahern wrote:
> > On 5/19/19 9:43 AM, Nathan Chancellor wrote:
> > > Hi all,
> > >
> > > This commit is causing issues on Android devices when Wi-Fi and mobile
> > > data are both enabled. The device will do a soft reboot consistently.
> > > So far, I've had reports on the Pixel 3 XL, OnePlus 6, Pocophone, and
> > > Note 9 and I can reproduce on my OnePlus 6.
> > >
> > > Sorry for taking so long to report this, I just figured out how to
> > > reproduce it today and I didn't want to report it without that.
> > >
> > > Attached is a full dmesg and the relevant snippet from Android's logcat.
> > >
> > > Let me know what I can do to help debug,
> > > Nathan
> > >
> >
> > It's a backport problem. err needs to be reset to 0 before the goto.
>
> Ah, I see it, let me go queue up a fix for this.
Here's the fix I'm queueing up now:
From b42f0ebbe4431ff7ce99c916555418f4a4c2be67 Mon Sep 17 00:00:00 2001
From: Greg Kroah-Hartman <[email protected]>
Date: Mon, 20 May 2019 11:07:29 +0200
Subject: [PATCH] fib_rules: fix error in backport of e9919a24d302 ("fib_rules:
return 0...")
When commit e9919a24d302 ("fib_rules: return 0 directly if an exactly
same rule exists when NLM_F_EXCL not supplied") was backported to 4.9.y,
it changed the logic a bit as err should have been reset before exiting
the test, like it happens in the original logic.
If this is not set, errors happen :(
Reported-by: Nathan Chancellor <[email protected]>
Reported-by: David Ahern <[email protected]>
Reported-by: Florian Westphal <[email protected]>
Cc: Hangbin Liu <[email protected]>
Cc: David S. Miller <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
net/core/fib_rules.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
index bb26457e8c21..c03dd2104d33 100644
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c
@@ -430,6 +430,7 @@ int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr *nlh)
goto errout_free;
if (rule_exists(ops, frh, tb, rule)) {
+ err = 0;
if (nlh->nlmsg_flags & NLM_F_EXCL)
err = -EEXIST;
goto errout_free;
--
2.21.0