ifnet: Move SIOCSIFVNET from ifhwioctl() to ifioctl()
ClosedPublic
Actions

Authored by zlei on Mon, Mar 16, 6:31 PM.

Details

Reviewers

jamie
kp
glebius

Group Reviewers

network

Commits

rG38bd7ef62f31: ifnet: Move SIOCSIFVNET from ifhwioctl() to ifioctl()

Summary

SIOCSIFVNET is not a hardware ioctl. Move it to where it belongs.

Where here, rewrite the logic of checking whether we are moving the
interface from and to the same vnet or not, since it is obviously not
safe to access the members of an interface until it is proven that the
interface is on the "active" list.

MFC after: 2 weeks

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

zlei created this revision.Mon, Mar 16, 6:31 PM

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald TranscriptMon, Mar 16, 6:31 PM

zlei requested review of this revision.Mon, Mar 16, 6:31 PM

zlei mentioned this in D55832: ifnet: if_vmove_(loan|reclaim): Refactor a bit the checking of src / dst vnet.Mon, Mar 16, 6:37 PM

zlei added inline comments.Mon, Mar 16, 6:43 PM

sys/net/if.c
1199	The error `ENXIO` means the interface does not exists. Should choose a better error to distinguish that with the case not found child prison / vnet.

Correct me if I am wrong, but this seems to be two separate changes. Can this be split please?

This revision now requires changes to proceed.Mon, Mar 16, 6:53 PM

In D55880#1278503, @glebius wrote:

Correct me if I am wrong, but this seems to be two separate changes. Can this be split please?

Ah I mixed the changes to the comments into this revision. I'll update.

Removed the changes to comments.

zlei added inline comments.Tue, Mar 17, 8:52 AM

sys/net/if.c
1192	This revision is based on latest main, so this removal is competing with D55875 . I'd personally prefer to land this first. With this change it is more robust to check "the moving between the same vnet".

Rebased on latest main.

Landed D55875 after running extensive tests.

@glebius Does it look good to you now ?

I have no objection to this change. And thanks for splitting the unrelated part into a separate change.

I'm resigning because I do not want to approve any improvements into SIOCSIFVNET. I'm convinced this was a bad idea from the very beginning. We should not invest time into improving it, we rather should invest time into proper full interface detach followed by full attach within a different jail/vnet context. Not yet fully convinced, but very likely we should only invest into netlink(4) API for doing that, not in ioctl(2).

In D55880#1289312, @glebius wrote:

I have no objection to this change. And thanks for splitting the unrelated part into a separate change.

I'm resigning because I do not want to approve any improvements into SIOCSIFVNET. I'm convinced this was a bad idea from the very beginning. We should not invest time into improving it, we rather should invest time into proper full interface detach followed by full attach within a different jail/vnet context. Not yet fully convinced, but very likely we should only invest into netlink(4) API for doing that, not in ioctl(2).

I see your point.

I think it is still valuable to make the logic clear and robust, given we still have stable branches to maintain.

proper full interface detach followed by full attach within a different jail/vnet context

I have an idea to disable vmove operations for some drivers as the first step, esp the loop(4) interface. Other candicates are gif(4), gre(4), wg(4), me(4), disc(4), edsc(4), bridge(4), enc(4), ipsec(4), lagg(4), ovpn(4), pflog(4), pfsync(4), stf(4). They're all cloneable.

Some I intended to keep them vmovable. Currently all physical interfaces, vlan(4), tun(4), tap(4), epair(4) and wlan(4).

The next step can be your proposal. For cloneable interfaces those depends on the physical interfaces, a new ioctl or netlink interface may introduced. They are create in child prison / vnet, and no vmove required.

The epair(4) is a bit tough. I have no idea what is a good ioctl for it.

The main concern to split those into steps is to make less POLA to end users. For example I do not expect a user want to vmove a loop(4) interface to another vnet.

dave_freedave.net added a subscriber: dave_freedave.net.Sun, Apr 12, 3:46 PM

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Apr 13, 4:40 AM

Closed by commit rG38bd7ef62f31: ifnet: Move SIOCSIFVNET from ifhwioctl() to ifioctl() (authored by zlei). · Explain Why

This revision was automatically updated to reflect the committed changes.

zlei added a commit: rG38bd7ef62f31: ifnet: Move SIOCSIFVNET from ifhwioctl() to ifioctl().