[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 1/2] net: tap: check if the file descriptor is valid befor
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v3 1/2] net: tap: check if the file descriptor is valid before using it |
Date: |
Mon, 6 Jul 2020 10:45:25 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
On 7/2/20 10:18 AM, Philippe Mathieu-Daudé wrote:
> On 7/2/20 10:17 AM, Laurent Vivier wrote:
>> On 02/07/2020 10:00, Philippe Mathieu-Daudé wrote:
>>> On 7/1/20 9:39 PM, 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.
>>>>
>>>> For instance:
>>>> # ip link add link virbr0 name macvtap0 type macvtap mode bridge
>>>> # ip link set macvtap0 up
>>>> # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1)
>>>> # qemu-system-x86_64 -machine q35 -device
>>>> pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP
>>>> (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9
>>>> (qemu) device_add
>>>> driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0
>>>> (qemu) device_del net0
>>>> (qemu) netdev_del hostnet0
>>>> (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9
>>>> qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock:
>>>> Assertion `f != -1' failed.
>>>> Aborted (core dumped)
>>>>
>>>> To avoid that, add a function, qemu_try_set_nonblock(), that allows to
>>>> report the
>>>> problem without crashing.
>>>>
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>> ---
>>>> include/qemu/sockets.h | 1 +
>>>> net/tap.c | 16 +++++++++---
>>>> util/oslib-posix.c | 26 +++++++++++++------
>>>> util/oslib-win32.c | 57 ++++++++++++++++++++++++------------------
>>>> 4 files changed, 64 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>>>> index 57cd049d6edd..7d1f8135767d 100644
>>>> --- a/include/qemu/sockets.h
>>>> +++ b/include/qemu/sockets.h
>>>> @@ -18,6 +18,7 @@ int qemu_accept(int s, struct sockaddr *addr, socklen_t
>>>> *addrlen);
>>>> int socket_set_cork(int fd, int v);
>>>> int socket_set_nodelay(int fd);
>>>> void qemu_set_block(int fd);
>>>> +int qemu_try_set_nonblock(int fd);
>>>> void qemu_set_nonblock(int fd);
>>>> int socket_set_fast_reuse(int fd);
>>>>
>>>> diff --git a/net/tap.c b/net/tap.c
>>>> index 6207f61f84ab..fb04c9044ce2 100644
>>>> --- a/net/tap.c
>>>> +++ b/net/tap.c
>>>> @@ -766,6 +766,7 @@ int net_init_tap(const Netdev *netdev, const char
>>>> *name,
>>>> Error *err = NULL;
>>>> const char *vhostfdname;
>>>> char ifname[128];
>>>> + int ret = 0;
>>>
>>> No need to zero-initialize.
>>>
>>> Otherwise:
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>
>>
>> I would agree with you but gcc doesn't:
>>
>> net/tap.c:769:9: error: ‘ret’ may be used uninitialized in this function
>> [-Werror=maybe-uninitialized]
>> 769 | int ret;
>> | ^~~
>
> I tried to outsmart GCC by manual review :/
IOW my R-b stands:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>>
>> if nfds == 0, we don't enter in the loop and ret is never set.
>>
>> Thanks,
>> Laurent
>>
>
[PATCH v3 2/2] net: detect errors from probing vnet hdr flag for TAP devices, Laurent Vivier, 2020/07/01
Re: [PATCH v3 0/2] net: tap: check file descriptor can be used, Laurent Vivier, 2020/07/06