| Message ID | 20201022064146.79873-1-keli@akamai.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] net: Properly typecast int values to set sk_max_pacing_rate | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | success | Errors and warnings before: 48 this patch: 48 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 17 lines checked |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 48 this patch: 48 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
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>
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>
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> >
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>
>
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!
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) &&
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(-) base-commit: 287d35405989cfe0090e3059f7788dc531879a8d