if_tun: check device name

[kib]

Summary

to avoid panic if the name already exists, which is possible with the interface renaming.

PR: 266999

[kevans]

Yeah, so I had considered this as well, but I'm wondering if we want to try a little harder in the ifconfig tun create case to find a unit number that will work- right now, we effectively brick that functionality completely because we'll keep allocating the same unr for it, which will then get punted with EEXIST here.

That said, it seems like we'd need a new unrhdr api to make that easier.

(edit: I clicked accept anyways because the above caveat is still better than the current panic)

[kib]

In D43001#980526, @kevans wrote:

Yeah, so I had considered this as well, but I'm wondering if we want to try a little harder in the ifconfig tun create case to find a unit number that will work- right now, we effectively brick that functionality completely because we'll keep allocating the same unr for it, which will then get punted with EEXIST here.

That said, it seems like we'd need a new unrhdr api to make that easier.

(edit: I clicked accept anyways because the above caveat is still better than the current panic)

I think the problem you describe is different. My patch is intended to handle user-induced panics where they can rename interfaces to arbitrary name, and then this cause an obvious problem with tun creating corresponding devfs entry. Any issues with races in allocation of run unit numbers require different considerations, as you said.

BTW, if you can formulate desired interface for unr(9), I might try to help.

[kevans]

In D43001#980529, @kib wrote:

In D43001#980526, @kevans wrote:

Yeah, so I had considered this as well, but I'm wondering if we want to try a little harder in the ifconfig tun create case to find a unit number that will work- right now, we effectively brick that functionality completely because we'll keep allocating the same unr for it, which will then get punted with EEXIST here.

That said, it seems like we'd need a new unrhdr api to make that easier.

(edit: I clicked accept anyways because the above caveat is still better than the current panic)

I think the problem you describe is different. My patch is intended to handle user-induced panics where they can rename interfaces to arbitrary name, and then this cause an obvious problem with tun creating corresponding devfs entry. Any issues with races in allocation of run unit numbers require different considerations, as you said.

Not even a race in this case; consider a scenario where you, e.g., ifconfig wg0 create name tun1

wg0 has now taken tun1; let's assume we don't have any tun interfaces yet. If we ifconfig tun create once, we'll alloc_unr() and create tun0 as a result. If we do it again, alloc_unr() will give us tun1, but we can't create tun1 because wg0's taken the name. Right now that's a panic, but with this patch we'll free the unr and a repeat of ifconfig tun create will try tun1 and fail again.

I think in either case we can punt on that, land this and follow-up with something better for that case.

BTW, if you can formulate desired interface for unr(9), I might try to help.

Thanks, I'll think on it a little bit. I think we really just want a alloc_unr_from(), but if we do that then I suspect we'll probably want to push unit auto-assignment into tun_create_device() so that we don't have to keep re-allocating tp while we figure out a unit number that will work.

[kib]

In D43001#980532, @kevans wrote:

In D43001#980529, @kib wrote:

In D43001#980526, @kevans wrote:

Yeah, so I had considered this as well, but I'm wondering if we want to try a little harder in the ifconfig tun create case to find a unit number that will work- right now, we effectively brick that functionality completely because we'll keep allocating the same unr for it, which will then get punted with EEXIST here.

That said, it seems like we'd need a new unrhdr api to make that easier.

(edit: I clicked accept anyways because the above caveat is still better than the current panic)

I think the problem you describe is different. My patch is intended to handle user-induced panics where they can rename interfaces to arbitrary name, and then this cause an obvious problem with tun creating corresponding devfs entry. Any issues with races in allocation of run unit numbers require different considerations, as you said.

Not even a race in this case; consider a scenario where you, e.g., ifconfig wg0 create name tun1

wg0 has now taken tun1; let's assume we don't have any tun interfaces yet. If we ifconfig tun create once, we'll alloc_unr() and create tun0 as a result. If we do it again, alloc_unr() will give us tun1, but we can't create tun1 because wg0's taken the name. Right now that's a panic, but with this patch we'll free the unr and a repeat of ifconfig tun create will try tun1 and fail again.

If the user is so malicious to explicitly rename an interface into tun1, then it is his duty to specify proper unit number when creating real tun interface. I do not have any sympathy for the case.

What I was after, is the case where user unknowingly renames his tun into a node name which already exists in devfs due to other driver' activity.

I think in either case we can punt on that, land this and follow-up with something better for that case.

BTW, if you can formulate desired interface for unr(9), I might try to help.

Thanks, I'll think on it a little bit. I think we really just want a alloc_unr_from(), but if we do that then I suspect we'll probably want to push unit auto-assignment into tun_create_device() so that we don't have to keep re-allocating tp while we figure out a unit number that will work.

[kevans]

In D43001#980533, @kib wrote:

In D43001#980532, @kevans wrote:

In D43001#980529, @kib wrote:

In D43001#980526, @kevans wrote:

Yeah, so I had considered this as well, but I'm wondering if we want to try a little harder in the ifconfig tun create case to find a unit number that will work- right now, we effectively brick that functionality completely because we'll keep allocating the same unr for it, which will then get punted with EEXIST here.

That said, it seems like we'd need a new unrhdr api to make that easier.

(edit: I clicked accept anyways because the above caveat is still better than the current panic)

I think the problem you describe is different. My patch is intended to handle user-induced panics where they can rename interfaces to arbitrary name, and then this cause an obvious problem with tun creating corresponding devfs entry. Any issues with races in allocation of run unit numbers require different considerations, as you said.

Not even a race in this case; consider a scenario where you, e.g., ifconfig wg0 create name tun1

wg0 has now taken tun1; let's assume we don't have any tun interfaces yet. If we ifconfig tun create once, we'll alloc_unr() and create tun0 as a result. If we do it again, alloc_unr() will give us tun1, but we can't create tun1 because wg0's taken the name. Right now that's a panic, but with this patch we'll free the unr and a repeat of ifconfig tun create will try tun1 and fail again.

If the user is so malicious to explicitly rename an interface into tun1, then it is his duty to specify proper unit number when creating real tun interface. I do not have any sympathy for the case.

Fair enough