Re: [PATCH] net: tap: check if the file descriptor is valid before using it

[Jason Wang]

On 2020/6/30 下午6:35, Laurent Vivier wrote:
> On 30/06/2020 12:03, Jason Wang wrote:
> > On 2020/6/30 下午5:45, Laurent Vivier wrote:
> > > On 30/06/2020 11:31, Daniel P. Berrangé wrote:
> > > > On Tue, Jun 30, 2020 at 10:23:18AM +0100, Daniel P. Berrangé wrote:
> > > > > On Tue, Jun 30, 2020 at 05:21:49PM +0800, Jason Wang wrote:
> > > > > > On 2020/6/30 上午3:30, Laurent Vivier wrote:
> > > > > > > On 28/06/2020 08:31, Jason Wang wrote:
> > > > > > > > On 2020/6/25 下午7:56, Laurent Vivier wrote:
> > > > > > > > > On 25/06/2020 10:48, Daniel P. Berrangé wrote:
> > > > > > > > > > On Wed, Jun 24, 2020 at 09:00:09PM +0200, Laurent Vivier wrote:
> > > > > > > > > > > qemu_set_nonblock() checks that the file descriptor can be
> > > > > > > > > > > used and, if
> > > > > > > > > > > not, crashes QEMU. An assert() is used for that. The use of
> > > > > > > > > > > assert() is
> > > > > > > > > > > used to detect programming error and the coredump will allow
> > > > > > > > > > > to debug
> > > > > > > > > > > the problem.
> > > > > > > > > > >
> > > > > > > > > > > But in the case of the tap device, this assert() can be
> > > > > > > > > > > triggered by
> > > > > > > > > > > a misconfiguration by the user. At startup, it's not a real
> > > > > > > > > > > problem,
> > > > > > > > > > > but it
> > > > > > > > > > > can also happen during the hot-plug of a new device, and here
> > > > > > > > > > > it's a
> > > > > > > > > > > problem because we can crash a perfectly healthy system.
> > > > > > > > > > If the user/mgmt app is not correctly passing FDs, then there's
> > > > > > > > > > a whole
> > > > > > > > > > pile of bad stuff that can happen. Checking whether the FD is
> > > > > > > > > > valid is
> > > > > > > > > > only going to catch a small subset. eg consider if fd=9 refers
> > > > > > > > > > to the
> > > > > > > > > > FD that is associated with the root disk QEMU has open. We'll
> > > > > > > > > > fail to
> > > > > > > > > > setup the TAP device and close this FD, breaking the healthy
> > > > > > > > > > system
> > > > > > > > > > again.
> > > > > > > > > >
> > > > > > > > > > I'm not saying we can't check if the FD is valid, but lets be
> > > > > > > > > > clear that
> > > > > > > > > > this is not offering very much protection against a broken mgmt
> > > > > > > > > > apps
> > > > > > > > > > passing bad FDs.
> > > > > > > > > I agree with you, but my only goal here is to avoid the crash in
> > > > > > > > > this
> > > > > > > > > particular case.
> > > > > > > > >
> > > > > > > > > The punishment should fit the crime.
> > > > > > > > >
> > > > > > > > > The user can think the netdev_del doesn't close the fd, and he
> > > > > > > > > can try
> > > > > > > > > to reuse it. Sending back an error is better than crashing his
> > > > > > > > > system.
> > > > > > > > > After that, if the system crashes, it will be for the good
> > > > > > > > > reasons, not
> > > > > > > > > because of an assert.
> > > > > > > > Yes. And on top of this we may try to validate the TAP via st_dev
> > > > > > > > through fstat[1].
> > > > > > > I agree, but the problem I have is to know which major(st_dev) we can
> > > > > > > allow to use.
> > > > > > >
> > > > > > > Do we allow only macvtap major number?
> > > > > > Macvtap and tuntap.
> > > > > >
> > > > > >
> > > > > > > How to know the macvtap major number at user level?
> > > > > > > [it is allocated dynamically: do we need to parse /proc/devices?]
> > > > > > I think we can get them through fstat for /dev/net/tun and
> > > > > > /dev/macvtapX.
> > > > > Don't assume QEMU has any permission to access to these device nodes,
> > > > > only the pre-opened FDs it is given by libvirt.
> > > > Actually permissions are the least of the problem - the device nodes
> > > > won't even exist, because QEMU's almost certainly running in a private
> > > > mount namespace with a minimal /dev populated
> > > I'm working on a solution using /proc/devices.
> >
> > Similar issue with /dev. There's no guarantee that qemu can access
> > /proc/devices or it may not exist (CONFIG_PROCFS).
> There is a lot of things that will not work without /proc (several tools
> rely on /proc, like ps, top, lsof, mount, ...). Some information are
> only available from /proc, and if /proc is there, I think /proc/devices
> is always readable by everyone. Moreover /proc is already used by qemu
> in several places.
>
> It can also a best effort check.


Right.


> The problem with fstat() on /dev files is to guess the /dev/macvtapX as
> X varies (the same with /dev/tapY)..
>
> > > macvtap has its own major number, but tuntap use "misc" (10) major
> > > number.
> Another question: it is possible to use the "fd=" parameter with macvtap
> as macvtap creates a /dev/tapY device,


Yes.


> but how to do that with tuntap
> that does not create a /dev/tapY device?


I think there's no specific reason, it's just because it was wrote like 
that since the first version which is about 20 years ago.

Thanks


> Thanks,
> Laurent