When net_init_tap() succeeds for a multi-queue device, it returns a
non-zero ret=1 code to its caller, because of this code where ret becomes
1 when g_unix_set_fd_nonblocking succeeds. Luckily, the only current call
site checks for negative, rather than non-zero.
ret = g_unix_set_fd_nonblocking(fd, true, NULL);
if (!ret) {
...
goto free_fail;
Also, if g_unix_set_fd_nonblocking fails (though unlikely), ret=0 is returned,
and the caller will use a broken interface.
Fixes: a8208626ba89.. ("net: replace qemu_set_nonblock()")
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
net/tap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 1bf085d..1f3e927 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -906,8 +906,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
goto free_fail;
}
- ret = g_unix_set_fd_nonblocking(fd, true, NULL);
- if (!ret) {
+ if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
+ ret = -1;
error_setg_errno(errp, errno, "%s: Can't use file descriptor %d",
name, fd);
goto free_fail;
--
1.8.3.1On 4/4/23 18:00, Steve Sistare wrote:
> When net_init_tap() succeeds for a multi-queue device, it returns a
> non-zero ret=1 code to its caller, because of this code where ret becomes
Indeed g_unix_set_fd_nonblocking() returns TRUE on success.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 1 when g_unix_set_fd_nonblocking succeeds. Luckily, the only current call
> site checks for negative, rather than non-zero.
>
> ret = g_unix_set_fd_nonblocking(fd, true, NULL);
> if (!ret) {
> ...
> goto free_fail;
>
> Also, if g_unix_set_fd_nonblocking fails (though unlikely), ret=0 is returned,
> and the caller will use a broken interface.
We should return -1 from free_fail, not trying to propagate 'ret':
-- >8 --
diff --git a/net/tap.c b/net/tap.c
index 1bf085d422..e59238bda0 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -821,3 +821,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
char ifname[128];
- int ret = 0;
@@ -896,3 +895,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
"the number of vhostfds passed");
- ret = -1;
goto free_fail;
@@ -904,3 +902,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
if (fd == -1) {
- ret = -1;
goto free_fail;
@@ -908,4 +905,3 @@ int net_init_tap(const Netdev *netdev, const char *name,
- ret = g_unix_set_fd_nonblocking(fd, true, NULL);
- if (!ret) {
+ if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
error_setg_errno(errp, errno, "%s: Can't use file
descriptor %d",
@@ -918,3 +914,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
if (vnet_hdr < 0) {
- ret = -1;
goto free_fail;
@@ -924,3 +919,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
"vnet_hdr not consistent across given tap
fds");
- ret = -1;
goto free_fail;
@@ -934,3 +928,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
error_propagate(errp, err);
- ret = -1;
goto free_fail;
@@ -948,3 +941,3 @@ free_fail:
g_free(vhost_fds);
- return ret;
+ return -1;
} else if (tap->helper) {
---
> Fixes: a8208626ba89.. ("net: replace qemu_set_nonblock()")
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
> net/tap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
On 4/4/2023 6:00 PM, Philippe Mathieu-Daudé wrote:
> On 4/4/23 18:00, Steve Sistare wrote:
>> When net_init_tap() succeeds for a multi-queue device, it returns a
>> non-zero ret=1 code to its caller, because of this code where ret becomes
>
> Indeed g_unix_set_fd_nonblocking() returns TRUE on success.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>> 1 when g_unix_set_fd_nonblocking succeeds. Luckily, the only current call
>> site checks for negative, rather than non-zero.
>>
>> ret = g_unix_set_fd_nonblocking(fd, true, NULL);
>> if (!ret) {
>> ...
>> goto free_fail;
>>
>> Also, if g_unix_set_fd_nonblocking fails (though unlikely), ret=0 is returned,
>> and the caller will use a broken interface.
>
> We should return -1 from free_fail, not trying to propagate 'ret':
Thanks for the review. I will add your "return -1" changes if Jason agrees.
- Steve
>
> -- >8 --
> diff --git a/net/tap.c b/net/tap.c
> index 1bf085d422..e59238bda0 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -821,3 +821,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> char ifname[128];
> - int ret = 0;
>
> @@ -896,3 +895,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> "the number of vhostfds passed");
> - ret = -1;
> goto free_fail;
> @@ -904,3 +902,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> if (fd == -1) {
> - ret = -1;
> goto free_fail;
> @@ -908,4 +905,3 @@ int net_init_tap(const Netdev *netdev, const char *name,
>
> - ret = g_unix_set_fd_nonblocking(fd, true, NULL);
> - if (!ret) {
> + if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
> error_setg_errno(errp, errno, "%s: Can't use file descriptor %d",
> @@ -918,3 +914,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> if (vnet_hdr < 0) {
> - ret = -1;
> goto free_fail;
> @@ -924,3 +919,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> "vnet_hdr not consistent across given tap fds");
> - ret = -1;
> goto free_fail;
> @@ -934,3 +928,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> error_propagate(errp, err);
> - ret = -1;
> goto free_fail;
> @@ -948,3 +941,3 @@ free_fail:
> g_free(vhost_fds);
> - return ret;
> + return -1;
> } else if (tap->helper) {
> ---
>
>> Fixes: a8208626ba89.. ("net: replace qemu_set_nonblock()")
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>> net/tap.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
在 2023/4/5 23:38, Steven Sistare 写道:
> On 4/4/2023 6:00 PM, Philippe Mathieu-Daudé wrote:
>> On 4/4/23 18:00, Steve Sistare wrote:
>>> When net_init_tap() succeeds for a multi-queue device, it returns a
>>> non-zero ret=1 code to its caller, because of this code where ret becomes
>> Indeed g_unix_set_fd_nonblocking() returns TRUE on success.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>>> 1 when g_unix_set_fd_nonblocking succeeds. Luckily, the only current call
>>> site checks for negative, rather than non-zero.
>>>
>>> ret = g_unix_set_fd_nonblocking(fd, true, NULL);
>>> if (!ret) {
>>> ...
>>> goto free_fail;
>>>
>>> Also, if g_unix_set_fd_nonblocking fails (though unlikely), ret=0 is returned,
>>> and the caller will use a broken interface.
>> We should return -1 from free_fail, not trying to propagate 'ret':
> Thanks for the review. I will add your "return -1" changes if Jason agrees.
>
> - Steve
Note that the "free_fail" label is kind of ambiguous. It is used even if
we succeed if I was not wrong.
Thanks
>> -- >8 --
>> diff --git a/net/tap.c b/net/tap.c
>> index 1bf085d422..e59238bda0 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -821,3 +821,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> char ifname[128];
>> - int ret = 0;
>>
>> @@ -896,3 +895,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> "the number of vhostfds passed");
>> - ret = -1;
>> goto free_fail;
>> @@ -904,3 +902,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> if (fd == -1) {
>> - ret = -1;
>> goto free_fail;
>> @@ -908,4 +905,3 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>
>> - ret = g_unix_set_fd_nonblocking(fd, true, NULL);
>> - if (!ret) {
>> + if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
>> error_setg_errno(errp, errno, "%s: Can't use file descriptor %d",
>> @@ -918,3 +914,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> if (vnet_hdr < 0) {
>> - ret = -1;
>> goto free_fail;
>> @@ -924,3 +919,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> "vnet_hdr not consistent across given tap fds");
>> - ret = -1;
>> goto free_fail;
>> @@ -934,3 +928,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>> error_propagate(errp, err);
>> - ret = -1;
>> goto free_fail;
>> @@ -948,3 +941,3 @@ free_fail:
>> g_free(vhost_fds);
>> - return ret;
>> + return -1;
>> } else if (tap->helper) {
>> ---
>>
>>> Fixes: a8208626ba89.. ("net: replace qemu_set_nonblock()")
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>> net/tap.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
On 4/11/2023 2:32 AM, Jason Wang wrote:
> 在 2023/4/5 23:38, Steven Sistare 写道:
>> On 4/4/2023 6:00 PM, Philippe Mathieu-Daudé wrote:
>>> On 4/4/23 18:00, Steve Sistare wrote:
>>>> When net_init_tap() succeeds for a multi-queue device, it returns a
>>>> non-zero ret=1 code to its caller, because of this code where ret becomes
>>> Indeed g_unix_set_fd_nonblocking() returns TRUE on success.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>
>>>> 1 when g_unix_set_fd_nonblocking succeeds. Luckily, the only current call
>>>> site checks for negative, rather than non-zero.
>>>>
>>>> ret = g_unix_set_fd_nonblocking(fd, true, NULL);
>>>> if (!ret) {
>>>> ...
>>>> goto free_fail;
>>>>
>>>> Also, if g_unix_set_fd_nonblocking fails (though unlikely), ret=0 is returned,
>>>> and the caller will use a broken interface.
>>> We should return -1 from free_fail, not trying to propagate 'ret':
>> Thanks for the review. I will add your "return -1" changes if Jason agrees.
>
> Note that the "free_fail" label is kind of ambiguous. It is used even if we succeed if I was not wrong.
Yes, good catch. We must return 0 from free_fail on success.
I could delete all uses of ret, test errp in free_fail, and return either -1 or 0.
Or, you could accept my initial small patch.
What do you prefer?
- Steve
>>> -- >8 --
>>> diff --git a/net/tap.c b/net/tap.c
>>> index 1bf085d422..e59238bda0 100644
>>> --- a/net/tap.c
>>> +++ b/net/tap.c
>>> @@ -821,3 +821,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>> char ifname[128];
>>> - int ret = 0;
>>>
>>> @@ -896,3 +895,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>> "the number of vhostfds passed");
>>> - ret = -1;
>>> goto free_fail;
>>> @@ -904,3 +902,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>> if (fd == -1) {
>>> - ret = -1;
>>> goto free_fail;
>>> @@ -908,4 +905,3 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>>
>>> - ret = g_unix_set_fd_nonblocking(fd, true, NULL);
>>> - if (!ret) {
>>> + if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
>>> error_setg_errno(errp, errno, "%s: Can't use file descriptor %d",
>>> @@ -918,3 +914,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>> if (vnet_hdr < 0) {
>>> - ret = -1;
>>> goto free_fail;
>>> @@ -924,3 +919,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>> "vnet_hdr not consistent across given tap fds");
>>> - ret = -1;
>>> goto free_fail;
>>> @@ -934,3 +928,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>> error_propagate(errp, err);
>>> - ret = -1;
>>> goto free_fail;
>>> @@ -948,3 +941,3 @@ free_fail:
>>> g_free(vhost_fds);
>>> - return ret;
>>> + return -1;
>>> } else if (tap->helper) {
>>> ---
>>>
>>>> Fixes: a8208626ba89.. ("net: replace qemu_set_nonblock()")
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>> net/tap.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>
On Tue, Apr 11, 2023 at 9:10 PM Steven Sistare
<steven.sistare@oracle.com> wrote:
>
> On 4/11/2023 2:32 AM, Jason Wang wrote:
> > 在 2023/4/5 23:38, Steven Sistare 写道:
> >> On 4/4/2023 6:00 PM, Philippe Mathieu-Daudé wrote:
> >>> On 4/4/23 18:00, Steve Sistare wrote:
> >>>> When net_init_tap() succeeds for a multi-queue device, it returns a
> >>>> non-zero ret=1 code to its caller, because of this code where ret becomes
> >>> Indeed g_unix_set_fd_nonblocking() returns TRUE on success.
> >>>
> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>
> >>>> 1 when g_unix_set_fd_nonblocking succeeds. Luckily, the only current call
> >>>> site checks for negative, rather than non-zero.
> >>>>
> >>>> ret = g_unix_set_fd_nonblocking(fd, true, NULL);
> >>>> if (!ret) {
> >>>> ...
> >>>> goto free_fail;
> >>>>
> >>>> Also, if g_unix_set_fd_nonblocking fails (though unlikely), ret=0 is returned,
> >>>> and the caller will use a broken interface.
> >>> We should return -1 from free_fail, not trying to propagate 'ret':
> >> Thanks for the review. I will add your "return -1" changes if Jason agrees.
> >
> > Note that the "free_fail" label is kind of ambiguous. It is used even if we succeed if I was not wrong.
>
> Yes, good catch. We must return 0 from free_fail on success.
> I could delete all uses of ret, test errp in free_fail, and return either -1 or 0.
> Or, you could accept my initial small patch.
>
> What do you prefer?
I've queued your patch for 8.1.
Thanks
>
> - Steve
>
> >>> -- >8 --
> >>> diff --git a/net/tap.c b/net/tap.c
> >>> index 1bf085d422..e59238bda0 100644
> >>> --- a/net/tap.c
> >>> +++ b/net/tap.c
> >>> @@ -821,3 +821,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> >>> char ifname[128];
> >>> - int ret = 0;
> >>>
> >>> @@ -896,3 +895,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> >>> "the number of vhostfds passed");
> >>> - ret = -1;
> >>> goto free_fail;
> >>> @@ -904,3 +902,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> >>> if (fd == -1) {
> >>> - ret = -1;
> >>> goto free_fail;
> >>> @@ -908,4 +905,3 @@ int net_init_tap(const Netdev *netdev, const char *name,
> >>>
> >>> - ret = g_unix_set_fd_nonblocking(fd, true, NULL);
> >>> - if (!ret) {
> >>> + if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
> >>> error_setg_errno(errp, errno, "%s: Can't use file descriptor %d",
> >>> @@ -918,3 +914,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> >>> if (vnet_hdr < 0) {
> >>> - ret = -1;
> >>> goto free_fail;
> >>> @@ -924,3 +919,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> >>> "vnet_hdr not consistent across given tap fds");
> >>> - ret = -1;
> >>> goto free_fail;
> >>> @@ -934,3 +928,2 @@ int net_init_tap(const Netdev *netdev, const char *name,
> >>> error_propagate(errp, err);
> >>> - ret = -1;
> >>> goto free_fail;
> >>> @@ -948,3 +941,3 @@ free_fail:
> >>> g_free(vhost_fds);
> >>> - return ret;
> >>> + return -1;
> >>> } else if (tap->helper) {
> >>> ---
> >>>
> >>>> Fixes: a8208626ba89.. ("net: replace qemu_set_nonblock()")
> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >>>> ---
> >>>> net/tap.c | 4 ++--
> >>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >
© 2016 - 2024 Red Hat, Inc.