Date: Sat, 25 Oct 2014 16:15:57 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-arch@freebsd.org Subject: Re: syscalls from loadable modules compiled in statically into the kernel Message-ID: <20141025141557.GB20599@dft-labs.eu> In-Reply-To: <20141025132039.GA20599@dft-labs.eu> References: <20141025022808.GA14551@dft-labs.eu> <20141025092234.GI1877@kib.kiev.ua> <20141025132039.GA20599@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Oct 25, 2014 at 03:20:39PM +0200, Mateusz Guzik wrote:
> On Sat, Oct 25, 2014 at 12:22:34PM +0300, Konstantin Belousov wrote:
> > On Sat, Oct 25, 2014 at 04:28:09AM +0200, Mateusz Guzik wrote:
> > > The kernel has the following mechanism:
> > >
> > > int
> > > syscall_thread_enter(struct thread *td, struct sysent *se)
> > > {
> > > u_int32_t cnt, oldcnt;
> > >
> > > do {
> > > oldcnt = se->sy_thrcnt;
> > > if ((oldcnt & SY_THR_STATIC) != 0)
> > > return (0);
> > > if ((oldcnt & (SY_THR_DRAINING | SY_THR_ABSENT)) != 0)
> > > return (ENOSYS);
> > > cnt = oldcnt + SY_THR_INCR;
> > > } while (atomic_cmpset_acq_32(&se->sy_thrcnt, oldcnt, cnt) == 0);
> > > return (0);
> > > }
> > >
> > > Except it turns out that it is used even if given module (here: sysvshm) is
> > > compiled in statically.
> > >
> > > So my proposal is to give modules an easy way to tell whether they got
> > > compiled in and extend syscall_register interface so that it would allow
> > > registering static syscalls.
> > >
> > > The latter could also be used by modules which are loadable, but don't
> > > support unloads.
> > >
> > > I don't have any good idea how to provide aforementioned detection
> > > method though.
> > The method would be a combination of some change to syscall_register()
> > and #ifdef KLD_MODULE. Look at the sys/conf.h MAKEDEV_ETERNAL_KLD
> > definition, which provides similar in spirit optimization for
> > non-destructable cdevs.
> >
>
> Ok, so I'll add sysctl_register_flags and SY_THR_STATIC_KLD + making
> sure SY_THR_STATIC cannot be unregistered.
>
Turns out freebsd32 duplicates a lot of code and didn't receive some
fixes regular syscall table support got. I decided to just patch it up
without fixing that for now.
diff --git a/sys/compat/freebsd32/freebsd32_misc.c b/sys/compat/freebsd32/freebsd32_misc.c
index d909a71..8fe4b83 100644
--- a/sys/compat/freebsd32/freebsd32_misc.c
+++ b/sys/compat/freebsd32/freebsd32_misc.c
@@ -2627,9 +2627,13 @@ freebsd32_xxx(struct thread *td, struct freebsd32_xxx_args *uap)
#endif
int
-syscall32_register(int *offset, struct sysent *new_sysent,
- struct sysent *old_sysent)
+syscall32_register_flags(int *offset, struct sysent *new_sysent,
+ struct sysent *old_sysent, int flags)
{
+
+ if ((flags & ~SY_THR_STATIC) != 0)
+ return (EINVAL);
+
if (*offset == NO_SYSCALL) {
int i;
@@ -2648,15 +2652,26 @@ syscall32_register(int *offset, struct sysent *new_sysent,
*old_sysent = freebsd32_sysent[*offset];
freebsd32_sysent[*offset] = *new_sysent;
+ atomic_store_rel_32(&freebsd32_sysent[*offset].sy_thrcnt, flags);
return 0;
}
int
+syscall32_register(int *offset, struct sysent *new_sysent,
+ struct sysent *old_sysent)
+{
+
+ return (syscall32_register_flags(offset, new_sysent, old_sysent, 0));
+}
+
+int
syscall32_deregister(int *offset, struct sysent *old_sysent)
{
- if (*offset)
- freebsd32_sysent[*offset] = *old_sysent;
+ if (*offset == 0)
+ return (0);
+
+ freebsd32_sysent[*offset] = *old_sysent;
return 0;
}
@@ -2707,14 +2722,14 @@ syscall32_module_handler(struct module *mod, int what, void *arg)
}
int
-syscall32_helper_register(struct syscall_helper_data *sd)
+syscall32_helper_register_flags(struct syscall_helper_data *sd, int flags)
{
struct syscall_helper_data *sd1;
int error;
for (sd1 = sd; sd1->syscall_no != NO_SYSCALL; sd1++) {
- error = syscall32_register(&sd1->syscall_no, &sd1->new_sysent,
- &sd1->old_sysent);
+ error = syscall32_register_flags(&sd1->syscall_no,
+ &sd1->new_sysent, &sd1->old_sysent, flags);
if (error != 0) {
syscall32_helper_unregister(sd);
return (error);
@@ -2725,6 +2740,13 @@ syscall32_helper_register(struct syscall_helper_data *sd)
}
int
+syscall32_helper_register(struct syscall_helper_data *sd)
+{
+
+ return (syscall32_helper_register_flags(sd, 0));
+}
+
+int
syscall32_helper_unregister(struct syscall_helper_data *sd)
{
struct syscall_helper_data *sd1;
diff --git a/sys/compat/freebsd32/freebsd32_util.h b/sys/compat/freebsd32/freebsd32_util.h
index a5945cf..e473ef6 100644
--- a/sys/compat/freebsd32/freebsd32_util.h
+++ b/sys/compat/freebsd32/freebsd32_util.h
@@ -97,10 +97,14 @@ SYSCALL32_MODULE(syscallname, \
.syscall_no = FREEBSD32_SYS_##syscallname \
}
+int syscall32_register_flags(int *offset, struct sysent *new_sysent,
+ struct sysent *old_sysent, int flags);
int syscall32_register(int *offset, struct sysent *new_sysent,
struct sysent *old_sysent);
int syscall32_deregister(int *offset, struct sysent *old_sysent);
int syscall32_module_handler(struct module *mod, int what, void *arg);
+int syscall32_helper_register_flags(struct syscall_helper_data *sd,
+ int flags);
int syscall32_helper_register(struct syscall_helper_data *sd);
int syscall32_helper_unregister(struct syscall_helper_data *sd);
diff --git a/sys/kern/kern_syscalls.c b/sys/kern/kern_syscalls.c
index 03f6088..30dd203 100644
--- a/sys/kern/kern_syscalls.c
+++ b/sys/kern/kern_syscalls.c
@@ -104,11 +104,14 @@ syscall_thread_exit(struct thread *td, struct sysent *se)
}
int
-syscall_register(int *offset, struct sysent *new_sysent,
- struct sysent *old_sysent)
+syscall_register_flags(int *offset, struct sysent *new_sysent,
+ struct sysent *old_sysent, int flags)
{
int i;
+ if ((flags & ~SY_THR_STATIC) != 0)
+ return (EINVAL);
+
if (*offset == NO_SYSCALL) {
for (i = 1; i < SYS_MAXSYSCALL; ++i)
if (sysent[i].sy_call == (sy_call_t *)lkmnosys)
@@ -127,18 +130,31 @@ syscall_register(int *offset, struct sysent *new_sysent,
*old_sysent = sysent[*offset];
new_sysent->sy_thrcnt = SY_THR_ABSENT;
sysent[*offset] = *new_sysent;
- atomic_store_rel_32(&sysent[*offset].sy_thrcnt, 0);
+ atomic_store_rel_32(&sysent[*offset].sy_thrcnt, flags);
return (0);
}
int
+syscall_register(int *offset, struct sysent *new_sysent,
+ struct sysent *old_sysent)
+{
+
+ return (syscall_register_flags(offset, new_sysent, old_sysent, 0));
+}
+
+int
syscall_deregister(int *offset, struct sysent *old_sysent)
{
+ struct sysent *se;
- if (*offset) {
- syscall_thread_drain(&sysent[*offset]);
- sysent[*offset] = *old_sysent;
- }
+ if (*offset == 0)
+ return (0); /* XXX? */
+
+ se = &sysent[*offset];
+ if ((se->sy_thrcnt & SY_THR_STATIC) != 0)
+ return (EINVAL);
+ syscall_thread_drain(se);
+ sysent[*offset] = *old_sysent;
return (0);
}
@@ -190,14 +206,14 @@ syscall_module_handler(struct module *mod, int what, void *arg)
}
int
-syscall_helper_register(struct syscall_helper_data *sd)
+syscall_helper_register_flags(struct syscall_helper_data *sd, int flags)
{
struct syscall_helper_data *sd1;
int error;
for (sd1 = sd; sd1->syscall_no != NO_SYSCALL; sd1++) {
- error = syscall_register(&sd1->syscall_no, &sd1->new_sysent,
- &sd1->old_sysent);
+ error = syscall_register_flags(&sd1->syscall_no,
+ &sd1->new_sysent, &sd1->old_sysent, flags);
if (error != 0) {
syscall_helper_unregister(sd);
return (error);
@@ -208,6 +224,13 @@ syscall_helper_register(struct syscall_helper_data *sd)
}
int
+syscall_helper_register(struct syscall_helper_data *sd)
+{
+
+ return (syscall_helper_register_flags(sd, 0));
+}
+
+int
syscall_helper_unregister(struct syscall_helper_data *sd)
{
struct syscall_helper_data *sd1;
diff --git a/sys/kern/sysv_msg.c b/sys/kern/sysv_msg.c
index a572a0e..4fc04bc 100644
--- a/sys/kern/sysv_msg.c
+++ b/sys/kern/sysv_msg.c
@@ -252,11 +252,12 @@ msginit()
}
mtx_init(&msq_mtx, "msq", NULL, MTX_DEF);
- error = syscall_helper_register(msg_syscalls);
+ error = syscall_helper_register_flags(msg_syscalls, SY_THR_STATIC_KLD);
if (error != 0)
return (error);
#ifdef COMPAT_FREEBSD32
- error = syscall32_helper_register(msg32_syscalls);
+ error = syscall32_helper_register_flags(msg32_syscalls,
+ SY_THR_STATIC_KLD);
if (error != 0)
return (error);
#endif
diff --git a/sys/kern/sysv_sem.c b/sys/kern/sysv_sem.c
index c632902..dc1c66a 100644
--- a/sys/kern/sysv_sem.c
+++ b/sys/kern/sysv_sem.c
@@ -278,11 +278,12 @@ seminit(void)
semexit_tag = EVENTHANDLER_REGISTER(process_exit, semexit_myhook, NULL,
EVENTHANDLER_PRI_ANY);
- error = syscall_helper_register(sem_syscalls);
+ error = syscall_helper_register_flags(sem_syscalls, SY_THR_STATIC_KLD);
if (error != 0)
return (error);
#ifdef COMPAT_FREEBSD32
- error = syscall32_helper_register(sem32_syscalls);
+ error = syscall32_helper_register_flags(sem32_syscalls,
+ SY_THR_STATIC_KLD);
if (error != 0)
return (error);
#endif
diff --git a/sys/kern/sysv_shm.c b/sys/kern/sysv_shm.c
index 3480d11..b3132c3 100644
--- a/sys/kern/sysv_shm.c
+++ b/sys/kern/sysv_shm.c
@@ -910,11 +910,12 @@ shminit()
shmexit_hook = &shmexit_myhook;
shmfork_hook = &shmfork_myhook;
- error = syscall_helper_register(shm_syscalls);
+ error = syscall_helper_register_flags(shm_syscalls, SY_THR_STATIC_KLD);
if (error != 0)
return (error);
#ifdef COMPAT_FREEBSD32
- error = syscall32_helper_register(shm32_syscalls);
+ error = syscall32_helper_register_flags(shm32_syscalls,
+ SY_THR_STATIC_KLD);
if (error != 0)
return (error);
#endif
diff --git a/sys/netinet/sctp_syscalls.c b/sys/netinet/sctp_syscalls.c
index 3d0f549..8170ca8 100644
--- a/sys/netinet/sctp_syscalls.c
+++ b/sys/netinet/sctp_syscalls.c
@@ -94,11 +94,11 @@ sctp_syscalls_init(void *unused __unused)
{
int error;
- error = syscall_helper_register(sctp_syscalls);
+ error = syscall_helper_register_flags(sctp_syscalls, SY_THR_STATIC);
KASSERT((error == 0),
("%s: syscall_helper_register failed for sctp syscalls", __func__));
#ifdef COMPAT_FREEBSD32
- error = syscall32_helper_register(sctp_syscalls);
+ error = syscall32_helper_register_flags(sctp_syscalls, SY_THR_STATIC);
KASSERT((error == 0),
("%s: syscall32_helper_register failed for sctp syscalls",
__func__));
diff --git a/sys/sys/sysent.h b/sys/sys/sysent.h
index 0f1c256..12bc518 100644
--- a/sys/sys/sysent.h
+++ b/sys/sys/sysent.h
@@ -76,6 +76,12 @@ struct sysent { /* system call table */
#define SY_THR_ABSENT 0x4
#define SY_THR_INCR 0x8
+#ifdef KLD_MODULE
+#define SY_THR_STATIC_KLD 0
+#else
+#define SY_THR_STATIC_KLD 0x1
+#endif
+
struct image_params;
struct __sigset;
struct syscall_args;
@@ -241,10 +247,13 @@ struct syscall_helper_data {
.syscall_no = NO_SYSCALL \
}
+int syscall_register_flags(int *offset, struct sysent *new_sysent,
+ struct sysent *old_sysent, int flags);
int syscall_register(int *offset, struct sysent *new_sysent,
struct sysent *old_sysent);
int syscall_deregister(int *offset, struct sysent *old_sysent);
int syscall_module_handler(struct module *mod, int what, void *arg);
+int syscall_helper_register_flags(struct syscall_helper_data *sd, int flags);
int syscall_helper_register(struct syscall_helper_data *sd);
int syscall_helper_unregister(struct syscall_helper_data *sd);
> > >
> > > Also, please see https://reviews.freebsd.org/D1007 which moves
> > > SY_THR_STATIC check to an inline function, saving us 2 function calls on
> > > each syscall.
> >
> > Did you benchmarked this ? I dislike the code bloat.
>
> with syscall_timing from tools/tools ministat says +4% for getuid and +1
> for pipe+close.
>
> --
> Mateusz Guzik <mjguzik gmail.com>
--
Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141025141557.GB20599>