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 Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|