[libvirt] [PATCH 0/2] virSecurityManagerMetadataLock: Skip over duplicate paths
See 2/2 for explanation. Michal Prívozník (2): virSecurityManagerMetadataLock: Expand the comment on deadlocks virSecurityManagerMetadataLock: Skip over duplicate paths src/security/security_manager.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) -- 2.21.0
Document why we need to sort paths while it's still fresh in my memory. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index c205c3bf17..ade2c96141 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1289,7 +1289,12 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, if (VIR_ALLOC_N(fds, npaths) < 0) return NULL; - /* Sort paths to lock in order to avoid deadlocks. */ + /* Sort paths to lock in order to avoid deadlocks with other + * processes. For instance, if one process wants to lock + * paths A B and there's another that is trying to lock them + * in reversed order a deadlock might occur. But if we sort + * the paths alphabetically then both processes will try lock + * paths in the same order and thus no deadlock can occur. */ qsort(paths, npaths, sizeof(*paths), cmpstringp); for (i = 0; i < npaths; i++) { -- 2.21.0
On Thu, Jul 18, 2019 at 11:14:48AM +0200, Michal Privoznik wrote:
Document why we need to sort paths while it's still fresh in my memory.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
If there are two paths on the list that are the same we need to lock it only once. Because when we try to lock it the second time then open() fails. And if it didn't, locking it the second time would fail for sure. After all, it is sufficient to lock all paths just once satisfy the caller. Reported-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index ade2c96141..7c905f0785 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1294,16 +1294,35 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * paths A B and there's another that is trying to lock them * in reversed order a deadlock might occur. But if we sort * the paths alphabetically then both processes will try lock - * paths in the same order and thus no deadlock can occur. */ + * paths in the same order and thus no deadlock can occur. + * Lastly, it makes searching for duplicate paths below + * simpler. */ qsort(paths, npaths, sizeof(*paths), cmpstringp); for (i = 0; i < npaths; i++) { const char *p = paths[i]; struct stat sb; + size_t j; int retries = 10 * 1000; int fd; - if (!p || stat(p, &sb) < 0) + if (!p) + continue; + + /* If there's a duplicate path on the list, skip it over. + * Not only we would fail open()-ing it the second time, + * we would deadlock with ourselves trying to lock it the + * second time. After all, we've locked it when iterating + * over it the first time. */ + for (j = 0; j < i; j++) { + if (STREQ_NULLABLE(p, paths[j])) + break; + } + + if (i != j) + continue; + + if (stat(p, &sb) < 0) continue; if (S_ISDIR(sb.st_mode)) { -- 2.21.0
On Thu, Jul 18, 2019 at 11:14:49AM +0200, Michal Privoznik wrote:
If there are two paths on the list that are the same we need to lock it only once. Because when we try to lock it the second time then open() fails. And if it didn't, locking it the second time would fail for sure. After all, it is sufficient to lock all paths just once satisfy the caller.
Reported-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index ade2c96141..7c905f0785 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1294,16 +1294,35 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * paths A B and there's another that is trying to lock them * in reversed order a deadlock might occur. But if we sort * the paths alphabetically then both processes will try lock - * paths in the same order and thus no deadlock can occur. */ + * paths in the same order and thus no deadlock can occur. + * Lastly, it makes searching for duplicate paths below + * simpler. */ qsort(paths, npaths, sizeof(*paths), cmpstringp);
for (i = 0; i < npaths; i++) { const char *p = paths[i]; struct stat sb; + size_t j; int retries = 10 * 1000; int fd;
- if (!p || stat(p, &sb) < 0) + if (!p) + continue; + + /* If there's a duplicate path on the list, skip it over. + * Not only we would fail open()-ing it the second time, + * we would deadlock with ourselves trying to lock it the + * second time. After all, we've locked it when iterating + * over it the first time. */
Presumably this deals with the problem at cold boot. Is it possible to hit the same problem when we have cold plugged one device and then later try to hotplug another device using the same resource, or do the SRIOV assignment grouping requirements make the hotplug impossible ? 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 :|
On 7/18/19 11:28 AM, Daniel P. Berrangé wrote:
On Thu, Jul 18, 2019 at 11:14:49AM +0200, Michal Privoznik wrote:
If there are two paths on the list that are the same we need to lock it only once. Because when we try to lock it the second time then open() fails. And if it didn't, locking it the second time would fail for sure. After all, it is sufficient to lock all paths just once satisfy the caller.
Reported-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index ade2c96141..7c905f0785 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1294,16 +1294,35 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * paths A B and there's another that is trying to lock them * in reversed order a deadlock might occur. But if we sort * the paths alphabetically then both processes will try lock - * paths in the same order and thus no deadlock can occur. */ + * paths in the same order and thus no deadlock can occur. + * Lastly, it makes searching for duplicate paths below + * simpler. */ qsort(paths, npaths, sizeof(*paths), cmpstringp);
for (i = 0; i < npaths; i++) { const char *p = paths[i]; struct stat sb; + size_t j; int retries = 10 * 1000; int fd;
- if (!p || stat(p, &sb) < 0) + if (!p) + continue; + + /* If there's a duplicate path on the list, skip it over. + * Not only we would fail open()-ing it the second time, + * we would deadlock with ourselves trying to lock it the + * second time. After all, we've locked it when iterating + * over it the first time. */
Presumably this deals with the problem at cold boot. Is it possible to hit the same problem when we have cold plugged one device and then later try to hotplug another device using the same resource, or do the SRIOV assignment grouping requirements make the hotplug impossible ?
Okay, so digging deeper I think I know what the problem is. In general, it is of course possible to open a file multiple times, but that is not true for "/dev/vfio/N" which can be opened exactly one time. Indeed, looking into vfio_group_fops_open() in kernel.git/drivers/vfio/vfio.c if a group is already opened, then -EBUSY is returned (what we've seen in the error message). In fact, git log blames v3.11-rc1~46^2~1 which explicitly disallowed multiple open()-s. So, in theory, our code works, because we open() the files we are chown()-ing and close them immediatelly after. So the problem in which we try to lock /dev/vfio/N multiple times won't happen. However, since qemu already has the device opened, we will fail opening it. I can see two options here: 1) ignore this specific error in virSecurityManagerMetadataLock(). This will of course mean that the caller (virSecurityDACTransactionRun()) will chown() the /dev/vfio/N file without a lock, but then again, we have namespaces and there can't be any other domain using the path. And also, we don't really chown() if the file already has the correct owner - which is going to be 99.99% cases unless there would be different seclabels for two PCI devices (do we even allow <seclabel/> for <hostdev/>?). 2) Make virSecurityDACSetHostdevLabel() and virSecurityDACRestoreHostdevLabel() be NO-OP if a domain already/still has a device from the same IOMMU group like the one we're attaching/detaching. If these are NO-OPs then no relabel is done and thus the whole code I'm modifying in 1) is not even called. I have a preference for 1) because the code will be cleaner IMO. Michal
On 7/18/19 8:30 AM, Michal Privoznik wrote:
On 7/18/19 11:28 AM, Daniel P. Berrangé wrote:
On Thu, Jul 18, 2019 at 11:14:49AM +0200, Michal Privoznik wrote:
If there are two paths on the list that are the same we need to lock it only once. Because when we try to lock it the second time then open() fails. And if it didn't, locking it the second time would fail for sure. After all, it is sufficient to lock all paths just once satisfy the caller.
Reported-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index ade2c96141..7c905f0785 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1294,16 +1294,35 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * paths A B and there's another that is trying to lock them * in reversed order a deadlock might occur. But if we sort * the paths alphabetically then both processes will try lock - * paths in the same order and thus no deadlock can occur. */ + * paths in the same order and thus no deadlock can occur. + * Lastly, it makes searching for duplicate paths below + * simpler. */ qsort(paths, npaths, sizeof(*paths), cmpstringp); for (i = 0; i < npaths; i++) { const char *p = paths[i]; struct stat sb; + size_t j; int retries = 10 * 1000; int fd; - if (!p || stat(p, &sb) < 0) + if (!p) + continue; + + /* If there's a duplicate path on the list, skip it over. + * Not only we would fail open()-ing it the second time, + * we would deadlock with ourselves trying to lock it the + * second time. After all, we've locked it when iterating + * over it the first time. */
Presumably this deals with the problem at cold boot. Is it possible to hit the same problem when we have cold plugged one device and then later try to hotplug another device using the same resource, or do the SRIOV assignment grouping requirements make the hotplug impossible ?
Okay, so digging deeper I think I know what the problem is. In general, it is of course possible to open a file multiple times, but that is not true for "/dev/vfio/N" which can be opened exactly one time. Indeed, looking into vfio_group_fops_open() in kernel.git/drivers/vfio/vfio.c if a group is already opened, then -EBUSY is returned (what we've seen in the error message). In fact, git log blames v3.11-rc1~46^2~1 which explicitly disallowed multiple open()-s.
So, in theory, our code works, because we open() the files we are chown()-ing and close them immediatelly after. So the problem in which we try to lock /dev/vfio/N multiple times won't happen. However, since qemu already has the device opened, we will fail opening it.
I can see two options here:
1) ignore this specific error in virSecurityManagerMetadataLock(). This will of course mean that the caller (virSecurityDACTransactionRun()) will chown() the /dev/vfio/N file without a lock, but then again, we have namespaces and there can't be any other domain using the path. And also, we don't really chown() if the file already has the correct owner - which is going to be 99.99% cases unless there would be different seclabels for two PCI devices (do we even allow <seclabel/> for <hostdev/>?).
This option seems better to me because it already establishes an exception rule for files that can be opened only once. For now we found out about /dev/vfio/N files, but there might be more in the future, and then it is just a matter of adding more files to this exception rule. Thanks, DHB
2) Make virSecurityDACSetHostdevLabel() and virSecurityDACRestoreHostdevLabel() be NO-OP if a domain already/still has a device from the same IOMMU group like the one we're attaching/detaching. If these are NO-OPs then no relabel is done and thus the whole code I'm modifying in 1) is not even called.
I have a preference for 1) because the code will be cleaner IMO.
Michal
Thanks for the quick patches. They fixed the problem I was having with a domain using a PCI Multifunction card, 4 hostdevs that belongs to IOMMU group 1. The problem triggered when QEMU tried to open the path /dev/vfio/1 for the second time. For both patches: Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 7/18/19 6:14 AM, Michal Privoznik wrote:
See 2/2 for explanation.
Michal Prívozník (2): virSecurityManagerMetadataLock: Expand the comment on deadlocks virSecurityManagerMetadataLock: Skip over duplicate paths
src/security/security_manager.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)
participants (3)
-
Daniel Henrique Barboza -
Daniel P. Berrangé -
Michal Privoznik