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. macvtap has its own major number, but tuntap use "misc" (10) major number. Thanks, Laurent
