On Fri, 2024-09-06 at 10:18 +0200, Konrad Weihmann wrote:
> On 05.09.24 11:56, Richard Purdie wrote:
> > On Wed, 2024-09-04 at 12:21 +0000, Konrad Weihmann via
> > lists.openembedded.org wrote:
> > > in case of running two or more runqemu instances in parallel
> > > with no previously setup tap devices, the following happens:
> > >
> > > instance A probes for tap devices, but doesn't find
> > > any, proceeds to generating tap devices with the sudo call,
> > > resulting in tap0.
> > > instance B starts to probes, finds tap0.
> > > Both will lock tap0.
> > >
> > > tap0 will be then forwarded to qemu.
> > > Instance A reporting
> > >
> > > "Using preconfigured tap device tap0"
> > >
> > > but then failing with
> > >
> > > qemu-system... could not configure /dev/net/tun (tap0): Device or
> > > resource busy
> > >
> > > To fix that, lock the entire tap creation process with
> > > an exclusive file (blocking) lock, so only a single instance can
> > > perform the non-atomic changes.
> > >
> > > Signed-off-by: Konrad Weihmann <[email protected]>
> > > ---
> > > scripts/runqemu | 10 ++++++++++
> > > 1 file changed, 10 insertions(+)
> > >
> > > diff --git a/scripts/runqemu b/scripts/runqemu
> > > index 2817acb19f..e5db06be37 100755
> > > --- a/scripts/runqemu
> > > +++ b/scripts/runqemu
> > > @@ -1163,6 +1163,9 @@ to your build configuration.
> > >
> > > self.make_lock_dir(lockdir)
> > >
> > > + tap_setup_lock = open(os.path.join(lockdir,
> > > '_tap_creation_lock'), 'w')
> > > + fcntl.flock(tap_setup_lock, fcntl.LOCK_EX)
> > > +
> > > cmd = (ip, 'link')
> > > logger.debug('Running %s...' % str(cmd))
> > > ip_link = subprocess.check_output(cmd).decode('utf-8')
> > > @@ -1187,6 +1190,8 @@ to your build configuration.
> > >
> > > if not tap:
> > > if os.path.exists(nosudo_flag):
> > > + fcntl.flock(tap_setup_lock, fcntl.LOCK_UN)
> > > + tap_setup_lock.close()
> > > logger.error("Error: There are no available tap devices
> > > to use for networking,")
> > > logger.error("and I see %s exists, so I am not going to
> > > try creating" % nosudo_flag)
> > > raise RunQemuError("a new one with sudo.")
> >
> > This doesn't make sense to me. Surely you should be taking this lock
> > *within* the "if not tap" code block since that is where the ifup is
> > called out to with sudo? You appear to be dropping the lock there
> > instead?
>
> Right before the raise of an exception the lock should be released,
> shouldn't it?
Ok, I see why you've done it this way. I'd much prefer to move the lock
to just be in that conditional though. I don't believe it is needed in
the earlier search code - that code works fine today and is only used
if the devices are pre-setup.
> Otherwise we would rely on the fact that none of the calling functions
> does any exception handling and
> the entire process fails to release the lock.
>
> >
> > It appears you're putting the locking around the search for pre-
> > configured tap devices which is already atomic and working well from a
> > race perspective?
>
> That is the actual race condition here.
> On the on hand you have the search function, which in itself is atomic
> and save yes but then you have the setup function below
>
> 1198 try:
> 1199 tap =
> subprocess.check_output(cmd).decode('utf-8').strip()
> 1200 except subprocess.CalledProcessError as e:
> 1201 logger.error('Setting up tap device failed:\n%s\nRun
> runqemu-gen-tapdevs to manually create one.' % str(e))
> 1202 sys.exit(1)
> 1203 lockfile = os.path.join(lockdir, tap)
> 1204 self.taplock = lockfile + '.lock'
> 1205 self.acquire_taplock()
>
> the section from line 1199 till 1205 is completely unguarded from a lock
> perspective.
> Meaning (and I think that is what is actually happening) that the new
> tap device is created as the result of line 1199 by process A but locked
> by process B, as the search function finds it and acquires the lock.
> So in my opinion the entirely function needs to allow only a single
> instance to work on it (at least that is the most straight forward
> solution I can imagine).
> The only thing I couldn't figure out in the hundreds of test runs is,
> why the 1205 line lock isn't failing with an error, nevertheless this
> should be fixed and a function wide exclusive lock fixed the issue for me.
Looking at acquire_taplock(), it will print an error instead of an info
message and merrily continue on as the return value isn't checked. That
is perhaps the first key issue to fix as even with your change, that is
still a problem.
I think rather than add potentially fragile locking, we should try and
acquire the lock, if we can't, we should try and add yet another tap
device and continue until something hard errors...
Cheers,
Richard
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#204261):
https://lists.openembedded.org/g/openembedded-core/message/204261
Mute This Topic: https://lists.openembedded.org/mt/108263503/21656
Group Owner: [email protected]
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub
[[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
