diff mbox series

[net,v2] net: Properly typecast int values to set sk_max_pacing_rate

Message ID 20201022064146.79873-1-keli@akamai.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series [net,v2] net: Properly typecast int values to set sk_max_pacing_rate | expand

Commit Message

Li, Ke Oct. 22, 2020, 6:41 a.m. UTC 
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

Comments

Eric Dumazet Oct. 22, 2020, 7:48 a.m. UTC | #1 
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>
Li, Ke Oct. 22, 2020, 8:43 a.m. UTC | #2 
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>
Eric Dumazet Oct. 22, 2020, 2:19 p.m. UTC | #3 
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>
>
Li, Ke Oct. 22, 2020, 5:45 p.m. UTC | #4 
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>
    >
Jakub Kicinski Oct. 22, 2020, 7:25 p.m. UTC | #5 
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 mbox series

Patch

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) &&