* [PATCH net v2] net: Properly typecast int values to set sk_max_pacing_rate
@ 2020-10-22 6:41 Ke Li
2020-10-22 7:48 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Ke Li @ 2020-10-22 6:41 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, kli, Ke Li, Ji Li, Eric Dumazet
In setsockopt(SO_MAX_PACING_RATE) on 64bit systems, sk_max_pacing_rate,
after extended from 'u32' to 'unsigned long', takes unintentionally
hiked value whenever assigned from an 'int' value with MSB=1, due to
binary sign extension in promoting s32 to u64, e.g. 0x80000000 becomes
0xFFFFFFFF80000000.
Thus inflated sk_max_pacing_rate causes subsequent getsockopt to return
~0U unexpectedly. It may also result in increased pacing rate.
Fix by explicitly casting the 'int' value to 'unsigned int' before
assigning it to sk_max_pacing_rate, for zero extension to happen.
Fixes: 76a9ebe811fb ("net: extend sk_pacing_rate to unsigned long")
Signed-off-by: Ji Li <jli@akamai.com>
Signed-off-by: Ke Li <keli@akamai.com>
Cc: Eric Dumazet <edumazet@google.com>
---
v2: wrap the line in net/core/filter.c to less than 80 chars.
net/core/filter.c | 3 ++-
net/core/sock.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index c5e2a1c5fd8d..9370cd917fb9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4693,7 +4693,8 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
cmpxchg(&sk->sk_pacing_status,
SK_PACING_NONE,
SK_PACING_NEEDED);
- sk->sk_max_pacing_rate = (val == ~0U) ? ~0UL : val;
+ sk->sk_max_pacing_rate = (val == ~0U) ?
+ ~0UL : (unsigned int)val;
sk->sk_pacing_rate = min(sk->sk_pacing_rate,
sk->sk_max_pacing_rate);
break;
diff --git a/net/core/sock.c b/net/core/sock.c
index 4e8729357122..727ea1cc633c 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1163,7 +1163,7 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
case SO_MAX_PACING_RATE:
{
- unsigned long ulval = (val == ~0U) ? ~0UL : val;
+ unsigned long ulval = (val == ~0U) ? ~0UL : (unsigned int)val;
if (sizeof(ulval) != sizeof(val) &&
optlen >= sizeof(ulval) &&
base-commit: 287d35405989cfe0090e3059f7788dc531879a8d
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: Properly typecast int values to set sk_max_pacing_rate
2020-10-22 6:41 [PATCH net v2] net: Properly typecast int values to set sk_max_pacing_rate Ke Li
@ 2020-10-22 7:48 ` Eric Dumazet
2020-10-22 8:43 ` Li, Ke
2020-10-22 19:25 ` Jakub Kicinski
0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2020-10-22 7:48 UTC (permalink / raw)
To: Ke Li; +Cc: netdev, David Miller, Jakub Kicinski, kli, Ji Li
On Thu, Oct 22, 2020 at 8:42 AM Ke Li <keli@akamai.com> wrote:
>
> In setsockopt(SO_MAX_PACING_RATE) on 64bit systems, sk_max_pacing_rate,
> after extended from 'u32' to 'unsigned long', takes unintentionally
> hiked value whenever assigned from an 'int' value with MSB=1, due to
> binary sign extension in promoting s32 to u64, e.g. 0x80000000 becomes
> 0xFFFFFFFF80000000.
>
> Thus inflated sk_max_pacing_rate causes subsequent getsockopt to return
> ~0U unexpectedly. It may also result in increased pacing rate.
>
> Fix by explicitly casting the 'int' value to 'unsigned int' before
> assigning it to sk_max_pacing_rate, for zero extension to happen.
>
> Fixes: 76a9ebe811fb ("net: extend sk_pacing_rate to unsigned long")
> Signed-off-by: Ji Li <jli@akamai.com>
> Signed-off-by: Ke Li <keli@akamai.com>
> Cc: Eric Dumazet <edumazet@google.com>
> ---
> v2: wrap the line in net/core/filter.c to less than 80 chars.
SGTM (the other version was also fine, the 80 chars rule has been
relaxed/changed to 100 recently)
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: Properly typecast int values to set sk_max_pacing_rate
2020-10-22 7:48 ` Eric Dumazet
@ 2020-10-22 8:43 ` Li, Ke
2020-10-22 14:19 ` Eric Dumazet
2020-10-22 19:25 ` Jakub Kicinski
1 sibling, 1 reply; 6+ messages in thread
From: Li, Ke @ 2020-10-22 8:43 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David Miller, Jakub Kicinski, kli, Li, Ji
Thank you, Eric!
Nice to know the recent change to wrap-at-100. Will this be reflected somewhere, like, in Documentation/process/coding-style.rst?
Best,
-Ke
On 10/22/20, 12:49 AM, "Eric Dumazet" <edumazet@google.com> wrote:
On Thu, Oct 22, 2020 at 8:42 AM Ke Li <keli@akamai.com> wrote:
>
> In setsockopt(SO_MAX_PACING_RATE) on 64bit systems, sk_max_pacing_rate,
> after extended from 'u32' to 'unsigned long', takes unintentionally
> hiked value whenever assigned from an 'int' value with MSB=1, due to
> binary sign extension in promoting s32 to u64, e.g. 0x80000000 becomes
> 0xFFFFFFFF80000000.
>
> Thus inflated sk_max_pacing_rate causes subsequent getsockopt to return
> ~0U unexpectedly. It may also result in increased pacing rate.
>
> Fix by explicitly casting the 'int' value to 'unsigned int' before
> assigning it to sk_max_pacing_rate, for zero extension to happen.
>
> Fixes: 76a9ebe811fb ("net: extend sk_pacing_rate to unsigned long")
> Signed-off-by: Ji Li <jli@akamai.com>
> Signed-off-by: Ke Li <keli@akamai.com>
> Cc: Eric Dumazet <edumazet@google.com>
> ---
> v2: wrap the line in net/core/filter.c to less than 80 chars.
SGTM (the other version was also fine, the 80 chars rule has been
relaxed/changed to 100 recently)
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: Properly typecast int values to set sk_max_pacing_rate
2020-10-22 8:43 ` Li, Ke
@ 2020-10-22 14:19 ` Eric Dumazet
2020-10-22 17:45 ` Li, Ke
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2020-10-22 14:19 UTC (permalink / raw)
To: Li, Ke; +Cc: netdev, David Miller, Jakub Kicinski, kli, Li, Ji
On Thu, Oct 22, 2020 at 10:43 AM Li, Ke <keli@akamai.com> wrote:
>
> Thank you, Eric!
>
> Nice to know the recent change to wrap-at-100. Will this be reflected somewhere, like, in Documentation/process/coding-style.rst?
>
commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144
Author: Joe Perches <joe@perches.com>
Date: Fri May 29 16:12:21 2020 -0700
checkpatch/coding-style: deprecate 80-column warning
Yes, staying withing 80 columns is certainly still _preferred_. But
it's not the hard limit that the checkpatch warnings imply, and other
concerns can most certainly dominate.
Increase the default limit to 100 characters. Not because 100
characters is some hard limit either, but that's certainly a "what are
you doing" kind of value and less likely to be about the occasional
slightly longer lines.
Miscellanea:
- to avoid unnecessary whitespace changes in files, checkpatch will no
longer emit a warning about line length when scanning files unless
--strict is also used
- Add a bit to coding-style about alignment to open parenthesis
Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Best,
> -Ke
>
> On 10/22/20, 12:49 AM, "Eric Dumazet" <edumazet@google.com> wrote:
>
> On Thu, Oct 22, 2020 at 8:42 AM Ke Li <keli@akamai.com> wrote:
> >
> > In setsockopt(SO_MAX_PACING_RATE) on 64bit systems, sk_max_pacing_rate,
> > after extended from 'u32' to 'unsigned long', takes unintentionally
> > hiked value whenever assigned from an 'int' value with MSB=1, due to
> > binary sign extension in promoting s32 to u64, e.g. 0x80000000 becomes
> > 0xFFFFFFFF80000000.
> >
> > Thus inflated sk_max_pacing_rate causes subsequent getsockopt to return
> > ~0U unexpectedly. It may also result in increased pacing rate.
> >
> > Fix by explicitly casting the 'int' value to 'unsigned int' before
> > assigning it to sk_max_pacing_rate, for zero extension to happen.
> >
> > Fixes: 76a9ebe811fb ("net: extend sk_pacing_rate to unsigned long")
> > Signed-off-by: Ji Li <jli@akamai.com>
> > Signed-off-by: Ke Li <keli@akamai.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > ---
> > v2: wrap the line in net/core/filter.c to less than 80 chars.
>
> SGTM (the other version was also fine, the 80 chars rule has been
> relaxed/changed to 100 recently)
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: Properly typecast int values to set sk_max_pacing_rate
2020-10-22 14:19 ` Eric Dumazet
@ 2020-10-22 17:45 ` Li, Ke
0 siblings, 0 replies; 6+ messages in thread
From: Li, Ke @ 2020-10-22 17:45 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David Miller, Jakub Kicinski, kli, Li, Ji
Thanks for the info, Eric!
That explains why checkpatch.pl hadn't complained with v1 patch.
Best,
-Ke
On 10/22/20, 7:19 AM, "Eric Dumazet" <edumazet@google.com> wrote:
On Thu, Oct 22, 2020 at 10:43 AM Li, Ke <keli@akamai.com> wrote:
>
> Thank you, Eric!
>
> Nice to know the recent change to wrap-at-100. Will this be reflected somewhere, like, in Documentation/process/coding-style.rst?
>
commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144
Author: Joe Perches <joe@perches.com>
Date: Fri May 29 16:12:21 2020 -0700
checkpatch/coding-style: deprecate 80-column warning
Yes, staying withing 80 columns is certainly still _preferred_. But
it's not the hard limit that the checkpatch warnings imply, and other
concerns can most certainly dominate.
Increase the default limit to 100 characters. Not because 100
characters is some hard limit either, but that's certainly a "what are
you doing" kind of value and less likely to be about the occasional
slightly longer lines.
Miscellanea:
- to avoid unnecessary whitespace changes in files, checkpatch will no
longer emit a warning about line length when scanning files unless
--strict is also used
- Add a bit to coding-style about alignment to open parenthesis
Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Best,
> -Ke
>
> On 10/22/20, 12:49 AM, "Eric Dumazet" <edumazet@google.com> wrote:
>
> On Thu, Oct 22, 2020 at 8:42 AM Ke Li <keli@akamai.com> wrote:
> >
> > In setsockopt(SO_MAX_PACING_RATE) on 64bit systems, sk_max_pacing_rate,
> > after extended from 'u32' to 'unsigned long', takes unintentionally
> > hiked value whenever assigned from an 'int' value with MSB=1, due to
> > binary sign extension in promoting s32 to u64, e.g. 0x80000000 becomes
> > 0xFFFFFFFF80000000.
> >
> > Thus inflated sk_max_pacing_rate causes subsequent getsockopt to return
> > ~0U unexpectedly. It may also result in increased pacing rate.
> >
> > Fix by explicitly casting the 'int' value to 'unsigned int' before
> > assigning it to sk_max_pacing_rate, for zero extension to happen.
> >
> > Fixes: 76a9ebe811fb ("net: extend sk_pacing_rate to unsigned long")
> > Signed-off-by: Ji Li <jli@akamai.com>
> > Signed-off-by: Ke Li <keli@akamai.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > ---
> > v2: wrap the line in net/core/filter.c to less than 80 chars.
>
> SGTM (the other version was also fine, the 80 chars rule has been
> relaxed/changed to 100 recently)
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: Properly typecast int values to set sk_max_pacing_rate
2020-10-22 7:48 ` Eric Dumazet
2020-10-22 8:43 ` Li, Ke
@ 2020-10-22 19:25 ` Jakub Kicinski
1 sibling, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2020-10-22 19:25 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Ke Li, netdev, David Miller, kli, Ji Li
On Thu, 22 Oct 2020 09:48:48 +0200 Eric Dumazet wrote:
> On Thu, Oct 22, 2020 at 8:42 AM Ke Li <keli@akamai.com> wrote:
> >
> > In setsockopt(SO_MAX_PACING_RATE) on 64bit systems, sk_max_pacing_rate,
> > after extended from 'u32' to 'unsigned long', takes unintentionally
> > hiked value whenever assigned from an 'int' value with MSB=1, due to
> > binary sign extension in promoting s32 to u64, e.g. 0x80000000 becomes
> > 0xFFFFFFFF80000000.
> >
> > Thus inflated sk_max_pacing_rate causes subsequent getsockopt to return
> > ~0U unexpectedly. It may also result in increased pacing rate.
> >
> > Fix by explicitly casting the 'int' value to 'unsigned int' before
> > assigning it to sk_max_pacing_rate, for zero extension to happen.
> >
> > Fixes: 76a9ebe811fb ("net: extend sk_pacing_rate to unsigned long")
> > Signed-off-by: Ji Li <jli@akamai.com>
> > Signed-off-by: Ke Li <keli@akamai.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > ---
> > v2: wrap the line in net/core/filter.c to less than 80 chars.
>
> SGTM (the other version was also fine, the 80 chars rule has been
> relaxed/changed to 100 recently)
We went from old guidelines, to unclear guidelines, IDK which one is
worse :( Here the way the ternary expression was wrapping in a 80 char
window looked way less readable, so I thought I'd request a reformat.
> Reviewed-by: Eric Dumazet <edumazet@google.com>
Applied, thanks everyone!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-22 19:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-22 6:41 [PATCH net v2] net: Properly typecast int values to set sk_max_pacing_rate Ke Li
2020-10-22 7:48 ` Eric Dumazet
2020-10-22 8:43 ` Li, Ke
2020-10-22 14:19 ` Eric Dumazet
2020-10-22 17:45 ` Li, Ke
2020-10-22 19:25 ` Jakub Kicinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.