Re: [PATCH 2/2] dev_ioctl: split out SIOC?IFMAP ioctls

From: Arnd Bergmann <arnd@arndb.de>
To: Christoph Hellwig <hch@infradead.org>
Cc: "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Jiri Pirko <jiri@mellanox.com>,
	Taehee Yoo <ap420073@gmail.com>,
	Eric Dumazet <edumazet@google.com>,
	Alexei Starovoitov <ast@kernel.org>, Andrew Lunn <andrew@lunn.ch>,
	Jens Axboe <axboe@kernel.dk>, Networking <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] dev_ioctl: split out SIOC?IFMAP ioctls
Date: Fri, 25 Sep 2020 14:28:29 +0200	[thread overview]
Message-ID: <CAK8P3a0ht1c34K+4k3XxGvWA9cxWJSMNzQR2iYMcm98guMsj1A@mail.gmail.com> (raw)
In-Reply-To: <20200919054831.GN30063@infradead.org>

On Sat, Sep 19, 2020 at 7:48 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> > diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
> > index 797ba2c1562a..a332d6ae4dc6 100644
> > --- a/include/uapi/linux/if.h
> > +++ b/include/uapi/linux/if.h
> > @@ -247,7 +247,13 @@ struct ifreq {
> >               short   ifru_flags;
> >               int     ifru_ivalue;
> >               int     ifru_mtu;
> > +#ifndef __KERNEL__
> > +             /*
> > +              * ifru_map is rarely used but causes the incompatibility
> > +              * between native and compat mode.
> > +              */
> >               struct  ifmap ifru_map;
> > +#endif
>
> Do we need a way to verify that this never changes the struct size?

Not sure which way you would want to check. The point of the patch
is that it does change the struct size inside of the kernel but not
in user space.

Do you mean we should check that the (larger) user space size
remains what it is for future changes, or that the (smaller)
kernel size remains the same on all kernels, or maybe both?

> > +int dev_ifmap(struct net *net, struct ifreq __user *ifr, unsigned int cmd)
> > +{
> > +     struct net_device *dev;
> > +     char ifname[IFNAMSIZ];
> > +     char *colon;
> > +     struct compat_ifmap cifmap;
> > +     struct ifmap ifmap;
> > +     int ret;
> > +
> > +     if (copy_from_user(ifname, ifr->ifr_name, sizeof(ifname)))
> > +             return -EFAULT;
> > +     ifname[IFNAMSIZ-1] = 0;
> > +     colon = strchr(ifname, ':');
> > +     if (colon)
> > +             *colon = 0;
> > +     dev_load(net, ifname);
> > +
> > +     switch (cmd) {
> > +     case SIOCGIFMAP:
> > +             rcu_read_lock();
...
> > +             break;
> > +
> > +     case SIOCSIFMAP:
> > +             if (!capable(CAP_NET_ADMIN) ||
...
> > +             break;
> > +     }
> > +     return ret;
>
> I'd rather split this into a separate hepers for each ioctl command
> instead of having anothr multiplexer here, maybe with another helper
> for the common code.

Yes, good idea.

> I also find the rcu unlock inside the branches rather strange, but
> I can't think of a good alternative.

I could assign to the local 'struct ifmap' first under the lock, and
then only copy from there to 'struct compat_ifmap' without the lock
in the compat path. It's probably not better, but I'll give it a try.

The kernel test robot found a build regression with CONFIG_COMPAT
is disabled, I'm fixing that by moving the struct definition of the
global #ifdef in linux/compat.h, which seems nicer than adding
another #ifdef in dev_ifmap.

     Arnd