Re: [PATCH linux] net/ipv4: ping_group_range: allow GID from 2147483648 to 4294967294

[Akihiro Suda]

> I think we can use proc_doulongvec_minmax() instead of open coding.

Thanks, sent v2 with your suggestion
https://lore.kernel.org/lkml/168557950756.14226.6470993129419598644-0@xxxxxxxxx/

2023年6月1日(木) 8:00 Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>:
>
> From: Eric Dumazet <edumazet@xxxxxxxxxx>
> Date: Wed, 31 May 2023 23:09:02 +0200
> > On Wed, May 31, 2023 at 9:19 PM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
> > >
> > > From: ~akihirosuda <akihirosuda@xxxxxxxxx>
> > > Date: Wed, 31 May 2023 19:42:49 +0900
> > > > From: Akihiro Suda <akihiro.suda.cz@xxxxxxxxxxxxx>
> > > >
> > > > 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<---