Re: Unlock ip6_sysctl()
On 2023-05-18 00:14 +02, Alexander Bluhm wrote: > And why is this ip6_soiikey in the kernel anyway? I guess it is > from a time when address configuration was done in the kernel. > Could slaacd(8) just read /etc/soii.key? Originally we implemented RFC 7217 for link-local addresses, too. The value of doing that is IMO not very high and it turned out that there are ISPs out there that calculate the clients link-local address from the MAC address and route a /64 (or whatever) to that link-local address. If we calculate a different link-local address IPv6 is broken. It's pretty difficult to debug this so we removed support for it in the kernel. p.s. Furthermore I think link-local addresses should be generated by userland. -- In my defence, I have been left unsupervised.
Re: Unlock ip6_sysctl()
On Thu, May 18, 2023 at 01:56:13AM +0300, Vitaliy Makkoveev wrote: > > On 18 May 2023, at 01:14, Alexander Bluhm wrote: > > > > On Wed, May 17, 2023 at 12:46:02PM +0300, Vitaliy Makkoveev wrote: > >> Introduce `ip6_soiikey_lock' rwlock(9) to protect `ip6_soiikey'. It > >> accessed only by ip6_sysctl_soiikey() and ip6_sysctl() is the only > >> ip6_sysctl_soiikey() caller so context switch is allowed here. Also > >> remove unused `oldkey' from ip6_sysctl_soiikey(). > > > > rwlock or mutex? As sysctl mlocks userland memory, copyin/copyout > > cannot sleep here. So it should be safe to use a mutex. > > It is locked because context switch is strictly unwanted when you > do copyin/copyout within foreach loops of kernel lock protected > lists like ps_list. Really this lock is the kind of a hack. But > since rwlocks already introduced context switch to sysctl() paths, > we use sysctl_lock rwlock to prevent many processes to > simultaneously lock large amount of memory. This is another hack > in this layer. Guess, both of them will gone in the future, so > copyin/copyout will sleep. So rwlock is better. > > > > > And why is this ip6_soiikey in the kernel anyway? I guess it is > > from a time when address configuration was done in the kernel. > > Could slaacd(8) just read /etc/soii.key? > > It could, but this should be implemented in slaacd first. And this > work is not related to (*pr_sysctl)() unlocking. > Please stop changing the locking inside sysctl. It seems that the last change introduced a regression with the result that systems lock up. Until that is solved nothing else should be changed in this code. -- :wq Claudio
Re: Unlock ip6_sysctl()
> On 18 May 2023, at 01:14, Alexander Bluhm wrote:
>
> On Wed, May 17, 2023 at 12:46:02PM +0300, Vitaliy Makkoveev wrote:
>> Introduce `ip6_soiikey_lock' rwlock(9) to protect `ip6_soiikey'. It
>> accessed only by ip6_sysctl_soiikey() and ip6_sysctl() is the only
>> ip6_sysctl_soiikey() caller so context switch is allowed here. Also
>> remove unused `oldkey' from ip6_sysctl_soiikey().
>
> rwlock or mutex? As sysctl mlocks userland memory, copyin/copyout
> cannot sleep here. So it should be safe to use a mutex.
It is locked because context switch is strictly unwanted when you
do copyin/copyout within foreach loops of kernel lock protected
lists like ps_list. Really this lock is the kind of a hack. But
since rwlocks already introduced context switch to sysctl() paths,
we use sysctl_lock rwlock to prevent many processes to
simultaneously lock large amount of memory. This is another hack
in this layer. Guess, both of them will gone in the future, so
copyin/copyout will sleep. So rwlock is better.
>
> And why is this ip6_soiikey in the kernel anyway? I guess it is
> from a time when address configuration was done in the kernel.
> Could slaacd(8) just read /etc/soii.key?
It could, but this should be implemented in slaacd first. And this
work is not related to (*pr_sysctl)() unlocking.
>
>> For IPV6CTL_MRTSTATS, IPV6CTL_MRTMIF and IPV6CTL_MRTMFC cases netlock
>> could be relaxed to share netlock, but with separate diffs.
>>
>> ok?
>>
>> Index: sys/netinet6/in6_proto.c
>> ===
>> RCS file: /cvs/src/sys/netinet6/in6_proto.c,v
>> retrieving revision 1.112
>> diff -u -p -r1.112 in6_proto.c
>> --- sys/netinet6/in6_proto.c 23 Nov 2022 14:48:28 - 1.112
>> +++ sys/netinet6/in6_proto.c 17 May 2023 09:37:05 -
>> @@ -128,6 +128,7 @@ const struct protosw inet6sw[] = {
>> {
>> .pr_domain = &inet6domain,
>> .pr_protocol = IPPROTO_IPV6,
>> + .pr_flags = PR_MPSYSCTL,
>> .pr_init = ip6_init,
>> .pr_slowtimo = frag6_slowtimo,
>> .pr_sysctl = ip6_sysctl
>> Index: sys/netinet6/ip6_input.c
>> ===
>> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
>> retrieving revision 1.254
>> diff -u -p -r1.254 ip6_input.c
>> --- sys/netinet6/ip6_input.c 21 Aug 2022 14:15:55 - 1.254
>> +++ sys/netinet6/ip6_input.c 17 May 2023 09:37:05 -
>> @@ -118,6 +118,7 @@ struct niqueue ip6intrq = NIQUEUE_INITIA
>> struct cpumem *ip6counters;
>>
>> uint8_t ip6_soiikey[IP6_SOIIKEY_LEN];
>> +struct rwlock ip6_soiikey_lock = RWLOCK_INITIALIZER("soiikeylk");
>>
>> int ip6_ours(struct mbuf **, int *, int, int);
>> int ip6_check_rh0hdr(struct mbuf *, int *);
>> @@ -1477,17 +1478,16 @@ ip6_sysctl_ip6stat(void *oldp, size_t *o
>> int
>> ip6_sysctl_soiikey(void *oldp, size_t *oldlenp, void *newp, size_t newlen)
>> {
>> -uint8_t oldkey[IP6_SOIIKEY_LEN];
>> int error;
>>
>> error = suser(curproc);
>> if (error != 0)
>> return (error);
>>
>> -memcpy(oldkey, ip6_soiikey, sizeof(oldkey));
>> -
>> +rw_enter_write(&ip6_soiikey_lock);
>> error = sysctl_struct(oldp, oldlenp, newp, newlen, ip6_soiikey,
>> sizeof(ip6_soiikey));
>> +rw_exit_write(&ip6_soiikey_lock);
>>
>> return (error);
>> }
Re: Unlock ip6_sysctl()
On Wed, May 17, 2023 at 12:46:02PM +0300, Vitaliy Makkoveev wrote:
> Introduce `ip6_soiikey_lock' rwlock(9) to protect `ip6_soiikey'. It
> accessed only by ip6_sysctl_soiikey() and ip6_sysctl() is the only
> ip6_sysctl_soiikey() caller so context switch is allowed here. Also
> remove unused `oldkey' from ip6_sysctl_soiikey().
rwlock or mutex? As sysctl mlocks userland memory, copyin/copyout
cannot sleep here. So it should be safe to use a mutex.
And why is this ip6_soiikey in the kernel anyway? I guess it is
from a time when address configuration was done in the kernel.
Could slaacd(8) just read /etc/soii.key?
> For IPV6CTL_MRTSTATS, IPV6CTL_MRTMIF and IPV6CTL_MRTMFC cases netlock
> could be relaxed to share netlock, but with separate diffs.
>
> ok?
>
> Index: sys/netinet6/in6_proto.c
> ===
> RCS file: /cvs/src/sys/netinet6/in6_proto.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 in6_proto.c
> --- sys/netinet6/in6_proto.c 23 Nov 2022 14:48:28 - 1.112
> +++ sys/netinet6/in6_proto.c 17 May 2023 09:37:05 -
> @@ -128,6 +128,7 @@ const struct protosw inet6sw[] = {
> {
>.pr_domain = &inet6domain,
>.pr_protocol = IPPROTO_IPV6,
> + .pr_flags = PR_MPSYSCTL,
>.pr_init = ip6_init,
>.pr_slowtimo = frag6_slowtimo,
>.pr_sysctl = ip6_sysctl
> Index: sys/netinet6/ip6_input.c
> ===
> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> retrieving revision 1.254
> diff -u -p -r1.254 ip6_input.c
> --- sys/netinet6/ip6_input.c 21 Aug 2022 14:15:55 - 1.254
> +++ sys/netinet6/ip6_input.c 17 May 2023 09:37:05 -
> @@ -118,6 +118,7 @@ struct niqueue ip6intrq = NIQUEUE_INITIA
> struct cpumem *ip6counters;
>
> uint8_t ip6_soiikey[IP6_SOIIKEY_LEN];
> +struct rwlock ip6_soiikey_lock = RWLOCK_INITIALIZER("soiikeylk");
>
> int ip6_ours(struct mbuf **, int *, int, int);
> int ip6_check_rh0hdr(struct mbuf *, int *);
> @@ -1477,17 +1478,16 @@ ip6_sysctl_ip6stat(void *oldp, size_t *o
> int
> ip6_sysctl_soiikey(void *oldp, size_t *oldlenp, void *newp, size_t newlen)
> {
> - uint8_t oldkey[IP6_SOIIKEY_LEN];
> int error;
>
> error = suser(curproc);
> if (error != 0)
> return (error);
>
> - memcpy(oldkey, ip6_soiikey, sizeof(oldkey));
> -
> + rw_enter_write(&ip6_soiikey_lock);
> error = sysctl_struct(oldp, oldlenp, newp, newlen, ip6_soiikey,
> sizeof(ip6_soiikey));
> + rw_exit_write(&ip6_soiikey_lock);
>
> return (error);
> }
Unlock ip6_sysctl()
Introduce `ip6_soiikey_lock' rwlock(9) to protect `ip6_soiikey'. It
accessed only by ip6_sysctl_soiikey() and ip6_sysctl() is the only
ip6_sysctl_soiikey() caller so context switch is allowed here. Also
remove unused `oldkey' from ip6_sysctl_soiikey().
For IPV6CTL_MRTSTATS, IPV6CTL_MRTMIF and IPV6CTL_MRTMFC cases netlock
could be relaxed to share netlock, but with separate diffs.
ok?
Index: sys/netinet6/in6_proto.c
===
RCS file: /cvs/src/sys/netinet6/in6_proto.c,v
retrieving revision 1.112
diff -u -p -r1.112 in6_proto.c
--- sys/netinet6/in6_proto.c23 Nov 2022 14:48:28 - 1.112
+++ sys/netinet6/in6_proto.c17 May 2023 09:37:05 -
@@ -128,6 +128,7 @@ const struct protosw inet6sw[] = {
{
.pr_domain = &inet6domain,
.pr_protocol = IPPROTO_IPV6,
+ .pr_flags= PR_MPSYSCTL,
.pr_init = ip6_init,
.pr_slowtimo = frag6_slowtimo,
.pr_sysctl = ip6_sysctl
Index: sys/netinet6/ip6_input.c
===
RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.254
diff -u -p -r1.254 ip6_input.c
--- sys/netinet6/ip6_input.c21 Aug 2022 14:15:55 - 1.254
+++ sys/netinet6/ip6_input.c17 May 2023 09:37:05 -
@@ -118,6 +118,7 @@ struct niqueue ip6intrq = NIQUEUE_INITIA
struct cpumem *ip6counters;
uint8_t ip6_soiikey[IP6_SOIIKEY_LEN];
+struct rwlock ip6_soiikey_lock = RWLOCK_INITIALIZER("soiikeylk");
int ip6_ours(struct mbuf **, int *, int, int);
int ip6_check_rh0hdr(struct mbuf *, int *);
@@ -1477,17 +1478,16 @@ ip6_sysctl_ip6stat(void *oldp, size_t *o
int
ip6_sysctl_soiikey(void *oldp, size_t *oldlenp, void *newp, size_t newlen)
{
- uint8_t oldkey[IP6_SOIIKEY_LEN];
int error;
error = suser(curproc);
if (error != 0)
return (error);
- memcpy(oldkey, ip6_soiikey, sizeof(oldkey));
-
+ rw_enter_write(&ip6_soiikey_lock);
error = sysctl_struct(oldp, oldlenp, newp, newlen, ip6_soiikey,
sizeof(ip6_soiikey));
+ rw_exit_write(&ip6_soiikey_lock);
return (error);
}
