Provide a mechanism for clone failure
Needs ReviewPublic
Actions

Authored by kevans on May 25 2020, 10:37 PM.

Details

Reviewers

Summary

A failure in make_dev*() can happen if the driver will just clean up gracefully on naming conflicts, which would leave the half-constructed (!SI_NAMED) device laying around and no path to release its unit number for reuse.

This patch does some minor refactoring to allow destroy_devl to be called in such a failure mode with the new KPI, clone_failed. SI_NAMED is still enforced for !failure cases, and an additional assert is added to make sure we didn't somehow end up with !SI_NAMED and children. Everything else in destroy_devl seems to be safe for the half-constructed device case.

A future patch will remove the unit number allocations from if_tuntap and force it to just rely on the clone framework for this.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 31303

Event Timeline

kevans created this revision.May 25 2020, 10:37 PM

Herald added subscribers: melifaro, ae, imp. · View Herald TranscriptMay 25 2020, 10:37 PM

kevans requested review of this revision.May 25 2020, 10:37 PM

Harbormaster completed remote builds in B31303: Diff 72258.May 25 2020, 10:37 PM

I would prefer that cloning was instead removed from devfs, instead of trying to make it working.

sys/kern/kern_conf.c
1425	This flag is abused at the place, to mean 'destroy_devl' called ? If yes, would it be better to use another flag, perhaps set both in destroy_dev_sched and destroy_dev ?
1437	Can you explain more why is this special handling needed ?
sys/net/if_tuntap.c
821	Why ?

In D25008#550757, @kib wrote:

I would prefer that cloning was instead removed from devfs, instead of trying to make it working.

It does kind of seem to cause more problems than it really solves; using unr(9) is not that hard and this doesn't save any real time/effort in creating cdevs.

sys/kern/kern_conf.c
1425	That would seem to be the case, yes; this was just pulled out of `clone_cleanup` below, so I'd need to dig into it a little bit.
1437	This change is probably wrong, now that I put more thought into it; this was previously unconditionally done, but that seemed somewhat silly because `destroy_devl` will already do the work in the case that we're calling it. If we've reached this path, `destroy_devl` has already been invoked and the removal will happen, making it unclear that we really need to bother removing the clone from the list here at all. It's not immediately obvious to me that the clonelist must be cleared when returning from `clone_cleanup()`
sys/net/if_tuntap.c
821	This is somewhat unrelated to the overall change, but demonstrates the problem. Someone reported on the mailing list that they can rename a tun device to a different unit (e.g. `ifconfig tun create name tun1`). While a silly example because they should've just created tun1 instead of cloning a tun and renaming it to tun1, to this point I've not attempted to forbid this because that's actually somewhat difficult to accomplish and perhaps administrator has a reasonable explanation for what they've done. This part of the change was to not panic but instead get the EEXIST when we've already created a device alias for tun1 -> tun0 from the above ifconfig invocation so that we can just alert them to the conflict, but doing so means that we'll never be able to reclaim tun1 if the attempt to once again `ifconfig tun create` and the cloner infrastructure creates the half-constructed device that we cannot finalize.

I'm toying with the idea of providing a simpler KPI to do this, tentative idea:

1.) Allocate a unrhdr in the cdevsw if d_flags requests it in prep_cdevsw; cdevsw must specify a d_minunit and a d_maxunit
2.) Provide dev_grab_unit() to allocate/reserve a new unit number
3.) Provide dev_ungrab_unit() to release a unit number for cases where the device will not be created
4.) ungrab unit numbers in destroy_dev*() as needed

Convert all current users of the clone framework to just grab_unit before creating the device and rip out the clone framework, because !SI_NAMED devices have little value if the unit's been reserved via call to dev_grab_unit()

Let me explain in more details what I do not like. As I understand, you have the tun interface index, and you are trying to consistently use it also for:

unit number of clone
as the numeric suffix for the device name

I do not see why do you need to encode tun interface number as the unit number. IMO all of this become much simpler if you create a device with make_dev_s() with the name as it should be, but use pointers from ifp to cdev and from cdev to ifp to manage the relations between devfs node and interface. I did not read tun code to claim that this is doable, but the approach generally sounds better to me.

In D25008#551444, @kib wrote:

Let me explain in more details what I do not like. As I understand, you have the tun interface index, and you are trying to consistently use it also for:

unit number of clone

as the numeric suffix for the device name

I do not see why do you need to encode tun interface number as the unit number. IMO all of this become much simpler if you create a device with make_dev_s() with the name as it should be, but use pointers from ifp to cdev and from cdev to ifp to manage the relations between devfs node and interface. I did not read tun code to claim that this is doable, but the approach generally sounds better to me.

The unit number itself isn't really a problem, it's that we hit a name conflict and the clone_* infrastructure can't adequately handle the case where the caller fails and can't bring the clone to completion. Since the only proper way to use the clone infrastructure is to get a unit number or device with clone_create() prior to make_dev*(), you effectively permanently leak the unit you tried after failure (from name collision) because kern_conf.c can't know whether failure means the caller won't try again. The caller would need to subsidize the clone infrastructure with its own tracking of 'this unit never actually got created', which somewhat defeats the purpose of using clone_create() in the first place.