Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kuniyuki Iwashima <kuniyu@amazon.com>
To:     <edumazet@google.com>
CC:     <akihiro.suda.cz@hco.ntt.co.jp>, <akihirosuda@git.sr.ht>,
        <davem@davemloft.net>, <kuba@kernel.org>, <kuniyu@amazon.com>,
        <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>,
        <pabeni@redhat.com>, <segoon@openwall.com>, <suda.kyoto@gmail.com>
Subject: Re: [PATCH linux] net/ipv4: ping_group_range: allow GID from 2147483648 to 4294967294
Date:   Wed, 31 May 2023 15:59:47 -0700
Message-ID: <20230531225947.38239-1-kuniyu@amazon.com>
In-Reply-To: <CANn89iK13jkbKXv-rKiUbTqMrk3KjVPGYH_Vv7FtJJ5pTdUAYQ@mail.gmail.com>
References: <CANn89iK13jkbKXv-rKiUbTqMrk3KjVPGYH_Vv7FtJJ5pTdUAYQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Eric Dumazet <edumazet@google.com>
Date: Wed, 31 May 2023 23:09:02 +0200
> On Wed, May 31, 2023 at 9:19 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > From: ~akihirosuda <akihirosuda@git.sr.ht>
> > Date: Wed, 31 May 2023 19:42:49 +0900
> > > From: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
> > >
> > > With this commit, all the GIDs ("0 4294967294") can be written to the
> > > "net.ipv4.ping_group_range" sysctl.
> > >
> > > Note that 4294967295 (0xffffffff) is an invalid GID (see gid_valid() in
> > > include/linux/uidgid.h), and an attempt to register this number will cause
> > > -EINVAL.
> > >
> > > Prior to this commit, only up to GID 2147483647 could be covered.
> > > Documentation/networking/ip-sysctl.rst had "0 4294967295" as an example
> > > value, but this example was wrong and causing -EINVAL.
> > >
> > > In the implementation, proc_dointvec_minmax is no longer used because it
> > > does not support numbers from 2147483648 to 4294967294.
> >
> > Good catch.
> >
> > I think we can use proc_doulongvec_minmax() instead of open coding.
> >
> > With the diff below:
> >
> > ---8<---
> > # sysctl -a | grep ping
> > net.ipv4.ping_group_range = 0   2147483647
> > # sysctl -w net.ipv4.ping_group_range="0 4294967295"
> > sysctl: setting key "net.ipv4.ping_group_range": Invalid argument
> > # sysctl -w net.ipv4.ping_group_range="0 4294967294"
> > net.ipv4.ping_group_range = 0 4294967294
> > # sysctl -a | grep ping
> > net.ipv4.ping_group_range = 0   4294967294
> > ---8<---
> >
> > ---8<---
> > diff --git a/include/net/ping.h b/include/net/ping.h
> > index 9233ad3de0ad..9b401b9a9d35 100644
> > --- a/include/net/ping.h
> > +++ b/include/net/ping.h
> > @@ -20,7 +20,7 @@
> >   * gid_t is either uint or ushort.  We want to pass it to
> >   * proc_dointvec_minmax(), so it must not be larger than MAX_INT
> >   */
> > -#define GID_T_MAX (((gid_t)~0U) >> 1)
> > +#define GID_T_MAX ((gid_t)~0U)
> >
> >  /* Compatibility glue so we can support IPv6 when it's compiled as a module */
> >  struct pingv6_ops {
> > diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> > index 6ae3345a3bdf..11d401958673 100644
> > --- a/net/ipv4/sysctl_net_ipv4.c
> > +++ b/net/ipv4/sysctl_net_ipv4.c
> > @@ -35,8 +35,8 @@ static int ip_ttl_max = 255;
> >  static int tcp_syn_retries_min = 1;
> >  static int tcp_syn_retries_max = MAX_TCP_SYNCNT;
> >  static int tcp_syn_linear_timeouts_max = MAX_TCP_SYNCNT;
> > -static int ip_ping_group_range_min[] = { 0, 0 };
> > -static int ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
> > +static unsigned long ip_ping_group_range_min[] = { 0, 0 };
> > +static unsigned long ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
> >  static u32 u32_max_div_HZ = UINT_MAX / HZ;
> >  static int one_day_secs = 24 * 3600;
> >  static u32 fib_multipath_hash_fields_all_mask __maybe_unused =
> > @@ -165,8 +165,8 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
> >                                  void *buffer, size_t *lenp, loff_t *ppos)
> >  {
> >         struct user_namespace *user_ns = current_user_ns();
> > +       unsigned long urange[2];
> >         int ret;
> > -       gid_t urange[2];
> >         kgid_t low, high;
> >         struct ctl_table tmp = {
> >                 .data = &urange,
> > @@ -179,7 +179,7 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
> >         inet_get_ping_group_range_table(table, &low, &high);
> >         urange[0] = from_kgid_munged(user_ns, low);
> >         urange[1] = from_kgid_munged(user_ns, high);
> > -       ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> > +       ret = proc_doulongvec_minmax(&tmp, write, buffer, lenp, ppos);
> >
> >         if (write && ret == 0) {
> >                 low = make_kgid(user_ns, urange[0]);
> > ---8<---
> 
> 
> Will this work on 32bit build ?

It worked at least on my i686 build and qemu.

---8<---
# uname -a
Linux (none) 6.4.0-rc3-00648-g75455b906d82-dirty #76 SMP PREEMPT_DYNAMIC Wed May 31 21:30:31 UTC 2023 i686 GNU/Linux
# sysctl -a | grep ping
net.ipv4.ping_group_range = 1	0
# sysctl -w net.ipv4.ping_group_range="0 4294967295"
sysctl: setting key "net.ipv4.ping_group_range": Invalid argument
# sysctl -w net.ipv4.ping_group_range="0 4294967294"
net.ipv4.ping_group_range = 0 4294967294
# sysctl -a | grep ping
net.ipv4.ping_group_range = 0	4294967294
---8<---