net: Fix handling of unmapped user pages in if_getgroup()

[markj]

Summary

We cannot call copyout() while in a net epoch section, unless the user
memory is wired. Use the global ifnet lock to synchronize access to the
list instead.

Reported by: emaste

[emaste]

LGTM.

Maybe this would be a bit clearer as

	if (ifgr->ifgr_len == 0) {
                	NET_EPOCH_ENTER();
                        CQ_STAILQ_FOREACH(...
                	NET_EPOCH_EXIT();
                        return (0);
        }

        vslock(...)

i.e. handle the len == 0 determine the size case first?

[markj]

Split the two cases completely.

[emaste]

Thanks, that's more clear to me.

We are already assigning len = ifgr->ifgr_len; so we could move it up and use it in the vslock call IMO.

sys/net/if.c

1486–1487

We can move this up and use it in the call to vslock

[glebius]

The ioctl path shall not use the epoch. The list is protected by IFNET_WLOCK(). Ideally the IFNET_WLOCK should become a sleepable lock. A temporary solution would be to keep IFNET_WLOCK() as is and allocate a temporary buffer to fill, then unlock and do copyout from this buffer. To make buffer size guessing easier we can track number of members in the V_ifg_head and try to allocate this size.

[markj]

In D57154#1309827, @glebius wrote:

The ioctl path shall not use the epoch.

Why? What about the sysctl path?

The list is protected by IFNET_WLOCK(). Ideally the IFNET_WLOCK should become a sleepable lock. A temporary solution would be to keep IFNET_WLOCK() as is and allocate a temporary buffer to fill, then unlock and do copyout from this buffer. To make buffer size guessing easier we can track number of members in the V_ifg_head and try to allocate this size.

The global ifnet lock is already a sleepable lock, so we can just wrap the whole function with it. Technically the list is protected by the in_addr lock, which is non-sleepable, but in practice all modifications to the list are done under the ifnet lock.

Allocating a separate buffer is racy and error-prone, I don't really like that approach.

While I'm generally ok with reducing use of net_epoch in the control path:

1. at least pf needs to be able to traverse the group list in the data path, so the list must be net_epoch safe,
2. SIOCGIFGROUP is probably a quite common operation, so should avoid acquiring a global lock,

so I don't see why it's so necessary to use a lock here.

[glebius]

In D57154#1309864, @markj wrote:

In D57154#1309827, @glebius wrote:

The ioctl path shall not use the epoch.

Why? What about the sysctl path?

To avoid patches like this :) So that we don't need to do virtual memory tricks for such a simple operation as getting list of names from kernel.

In general changing and reading configuration from userland should be synchronized by locks. Of course, simple cases like grabbing a single integer via sysctl may use epoch.

The list is protected by IFNET_WLOCK(). Ideally the IFNET_WLOCK should become a sleepable lock. A temporary solution would be to keep IFNET_WLOCK() as is and allocate a temporary buffer to fill, then unlock and do copyout from this buffer. To make buffer size guessing easier we can track number of members in the V_ifg_head and try to allocate this size.

The global ifnet lock is already a sleepable lock, so we can just wrap the whole function with it. Technically the list is protected by the in_addr lock, which is non-sleepable, but in practice all modifications to the list are done under the ifnet lock.

Allocating a separate buffer is racy and error-prone, I don't really like that approach.

While I'm generally ok with reducing use of net_epoch in the control path:

1. at least pf needs to be able to traverse the group list in the data path, so the list must be net_epoch safe,

The list already is epoch safe, isn't it? Does pf need to pull the entire contents of the list on the data path or just do a match of a name against the list?

2. SIOCGIFGROUP is probably a quite common operation, so should avoid acquiring a global lock,

so I don't see why it's so necessary to use a lock here.

Common at what rate? Few events per second I'd guess. As long as the data path doesn't acquire the lock, there is nothing wrong for SIOCGIFGROUP to acquire the lock. Most network configuration ioctls acquire locks.

Of course acquiring per-interface lock is better than global one and looks like in this case it is possible.

[markj]

In D57154#1309870, @glebius wrote:

In D57154#1309864, @markj wrote:

In D57154#1309827, @glebius wrote:

The ioctl path shall not use the epoch.

Why? What about the sysctl path?

To avoid patches like this :) So that we don't need to do virtual memory tricks for such a simple operation as getting list of names from kernel.

We do it regularly, usually it's spelled sysctl_wire_old_buffer().

In general changing and reading configuration from userland should be synchronized by locks. Of course, simple cases like grabbing a single integer via sysctl may use epoch.

epoch is for synchronizing access to objects with finite lifetimes, there's no reason at all to use it for reading an integer.

The list is protected by IFNET_WLOCK(). Ideally the IFNET_WLOCK should become a sleepable lock. A temporary solution would be to keep IFNET_WLOCK() as is and allocate a temporary buffer to fill, then unlock and do copyout from this buffer. To make buffer size guessing easier we can track number of members in the V_ifg_head and try to allocate this size.

The global ifnet lock is already a sleepable lock, so we can just wrap the whole function with it. Technically the list is protected by the in_addr lock, which is non-sleepable, but in practice all modifications to the list are done under the ifnet lock.

Allocating a separate buffer is racy and error-prone, I don't really like that approach.

While I'm generally ok with reducing use of net_epoch in the control path:

1. at least pf needs to be able to traverse the group list in the data path, so the list must be net_epoch safe,

The list already is epoch safe, isn't it? Does pf need to pull the entire contents of the list on the data path or just do a match of a name against the list?

I am not sure what exactly it is doing.

[zlei]

@glebius
Of course acquiring per-interface lock is better than global one and looks like in this case it is possible.

Yes, that is perfect.

@markj

I think the introduction of net epoch makes it a bit harder to maintain. Traditional lock based synchronization is much easier to understand.

SIOCGIFGROUP is probably a quite common operation, so should avoid acquiring a global lock,

Compared to the data path, the ioctl is typically a low frequent operation. The mind to serialize the ioctl operation comes while I was working on the ifnet detach / vmove racing issue. Acquiring a ( global / per-interface ) lock shall not hurt the performance.

so I don't see why it's so necessary to use a lock here.

It is not about necessary, but a simpler programming model to ease the maintain, from the design perspective.

[markj]

Use the global ifnet lock to protect the group list

[bms]

This is giving me 2003 flashbacks.