[PATCH 0/2] Two <interface type='direct'/> related fixes
Except not really. Only the first one is strictly related to that type of <interface/>. The other one fixes a regression introduced in 8.4.0 and it just so happens that I'm able to reproduce it 100% on my (SRIOV-less) machine. Michal Prívozník (2): virNetDevSaveNetConfig: Pass mode to virFileWriteStr() qemuBuildInterfaceConnect: Initialize @tapfd array src/qemu/qemu_command.c | 2 ++ src/util/virnetdev.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) -- 2.35.1
For some types of SRIOV interfaces we create a temporary file where the state of the interface is saved before we start modifying it. The file is used then to restore the original configuration when the interface is no longer associated with any guest. For writing the file virFileWriteStr() is used. However, it's given wrong argument: the last argument is supposed to be mode to create the file with but virNetDevSaveNetConfig() passes open(2) flags (O_CREAT|O_TRUNC|O_WRONLY). We need the file to be writable and readable by root only (0600). Therefore, pass that mode instead of gibberish. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 93f836cd92..6c6f6dee42 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1977,7 +1977,7 @@ virNetDevSaveNetConfig(const char *linkdev, int vf, if (!(fileStr = virJSONValueToString(configJSON, true))) return -1; - if (virFileWriteStr(filePath, fileStr, O_CREAT|O_TRUNC|O_WRONLY) < 0) { + if (virFileWriteStr(filePath, fileStr, 0600) < 0) { virReportSystemError(errno, _("Unable to preserve mac/vlan tag " "for device = %s, vf = %d"), linkdev, vf); return -1; -- 2.35.1
On Mon, Jun 13, 2022 at 15:18:13 +0200, Michal Privoznik wrote:
For some types of SRIOV interfaces we create a temporary file where the state of the interface is saved before we start modifying it. The file is used then to restore the original configuration when the interface is no longer associated with any guest. For writing the file virFileWriteStr() is used. However, it's given wrong argument: the last argument is supposed to be mode to create the file with but virNetDevSaveNetConfig() passes open(2) flags (O_CREAT|O_TRUNC|O_WRONLY). We need the file to be writable and readable by root only (0600). Therefore, pass that mode instead of gibberish.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On 6/13/22 9:18 AM, Michal Privoznik wrote:
For some types of SRIOV interfaces we create a temporary file where the state of the interface is saved before we start modifying it. The file is used then to restore the original configuration when the interface is no longer associated with any guest. For writing the file virFileWriteStr() is used. However, it's given wrong argument: the last argument is supposed to be mode to create the file with but virNetDevSaveNetConfig() passes open(2) flags (O_CREAT|O_TRUNC|O_WRONLY). We need the file to be writable and readable by root only (0600). Therefore, pass that mode instead of gibberish.
Wow. This may be in competition for the longest living "how did this ever work?" bug in the code :-/ Since my name was on the git blame for the most recent change to this line, I had to figure out if it was really me that had misunderstood/misused virFileWriteStr() so grievously (wouldn't be the first or the last time). What I found was that this code had been moved around by multiple different people (including me) since originally being included in new code all the way back in commit cbd8227ee in June 2011. Anyway, Peter has already acked it, but still Reviewed-by: Laine Stump <laine@redhat.com>
When creating a TAP interface we can end up with multiple FDs, each representing one queue. However, these FDs must be relabelled as they are then passed to QEMU. In case of qemuBuildInterfaceConnect() we allocate the array for the FDs and then let function corresponding to the <interface/> type to fill the array with FDs. When any of the functions meets an error, it's also responsible for closing previously opened FDs. However, the functions take a shortcut: iterate through each member of the array and close it (if it's non-negative). This assumes that the array is initialized to negative values, which use to be the case before rewrite in v8.4.0-rc1~170 but after it it's no longer the case. Subsequently, "random" FDs are closed (okay, not that random since the array is allocated via g_new0(), but hey - FD 0 is still valid FD and might be valuable, actually). Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index be20053c0d..ecfe6020f3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8736,6 +8736,8 @@ qemuBuildInterfaceConnect(virDomainObj *vm, size_t tapfdSize = net->driver.virtio.queues; g_autofree int *tapfd = g_new0(int, tapfdSize + 1); + memset(tapfd, -1, (tapfdSize + 1) * sizeof(*tapfd)); + if (tapfdSize == 0) tapfdSize = 1; -- 2.35.1
On 6/13/22 15:18, Michal Privoznik wrote:
When creating a TAP interface we can end up with multiple FDs, each representing one queue. However, these FDs must be relabelled as they are then passed to QEMU. In case of qemuBuildInterfaceConnect() we allocate the array for the FDs and then let function corresponding to the <interface/> type to fill the array with FDs. When any of the functions meets an error, it's also responsible for closing previously opened FDs. However, the functions take a shortcut: iterate through each member of the array and close it (if it's non-negative). This assumes that the array is initialized to negative values, which use to be the case before rewrite in v8.4.0-rc1~170 but after it it's no longer the case. Subsequently, "random" FDs are closed (okay, not that random since the array is allocated via g_new0(), but hey - FD 0 is still valid FD and might be valuable, actually).
Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578 Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
After some discussion in BZ I'm going to append: Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2075383#c18 Michal
On Mon, Jun 13, 2022 at 15:18:14 +0200, Michal Privoznik wrote:
When creating a TAP interface we can end up with multiple FDs, each representing one queue. However, these FDs must be relabelled as they are then passed to QEMU. In case of qemuBuildInterfaceConnect() we allocate the array for the FDs and then let function corresponding to the <interface/> type to fill the array with FDs. When any of the functions meets an error, it's also responsible for closing previously opened FDs. However, the functions take a shortcut: iterate through each member of the array and close it (if it's non-negative). This assumes that the array is initialized to negative values, which use to be the case before rewrite in v8.4.0-rc1~170 but after it it's no longer the case. Subsequently, "random" FDs are closed (okay, not that random since the array is allocated via g_new0(), but hey - FD 0 is still valid FD and might be valuable, actually).
Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index be20053c0d..ecfe6020f3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8736,6 +8736,8 @@ qemuBuildInterfaceConnect(virDomainObj *vm, size_t tapfdSize = net->driver.virtio.queues; g_autofree int *tapfd = g_new0(int, tapfdSize + 1);
+ memset(tapfd, -1, (tapfdSize + 1) * sizeof(*tapfd)); +
Reviewed-by: Peter Krempa <pkrempa@redhat.com> but I'm adding to my todo list to fix the offending functions :)
On 6/13/22 16:04, Peter Krempa wrote:
On Mon, Jun 13, 2022 at 15:18:14 +0200, Michal Privoznik wrote:
When creating a TAP interface we can end up with multiple FDs, each representing one queue. However, these FDs must be relabelled as they are then passed to QEMU. In case of qemuBuildInterfaceConnect() we allocate the array for the FDs and then let function corresponding to the <interface/> type to fill the array with FDs. When any of the functions meets an error, it's also responsible for closing previously opened FDs. However, the functions take a shortcut: iterate through each member of the array and close it (if it's non-negative). This assumes that the array is initialized to negative values, which use to be the case before rewrite in v8.4.0-rc1~170 but after it it's no longer the case. Subsequently, "random" FDs are closed (okay, not that random since the array is allocated via g_new0(), but hey - FD 0 is still valid FD and might be valuable, actually).
Fixes: 7a38d3946bc1a7ef0206f36dfe3dbf422fb8d578 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index be20053c0d..ecfe6020f3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8736,6 +8736,8 @@ qemuBuildInterfaceConnect(virDomainObj *vm, size_t tapfdSize = net->driver.virtio.queues; g_autofree int *tapfd = g_new0(int, tapfdSize + 1);
+ memset(tapfd, -1, (tapfdSize + 1) * sizeof(*tapfd)); +
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
but I'm adding to my todo list to fix the offending functions :)
Yeah, that might be worth fixing. Pushed thanks. Michal
participants (4)
-
Laine Stump -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa