| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 18 Mar 2007 13:31:54 -0700 (PDT)
From: Davide Libenzi <davidel@...ilserver.org>
To: Arnd Bergmann <arnd@...db.de>
cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Oleg Nesterov <oleg@...sign.ru>
Subject: Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...
On Sat, 17 Mar 2007, Arnd Bergmann wrote:
> > +asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t
> > sizemask) +{
> > + int error;
> > + unsigned long flags;
> > + sigset_t sigmask;
> > + struct signalfd_ctx *ctx;
> > + struct sighand_struct *sighand;
> > + struct file *file;
> > + struct inode *inode;
> > +
> > + error = -EINVAL;
> > + if (sizemask != sizeof(sigset_t) ||
> > + copy_from_user(&sigmask, user_mask, sizeof(sigmask)))
> > + goto err_exit;
>
> sizeof(sigset_t) may be different for native and 32-bit compat code.
> It would be good if you could handle sizemask==4 && sizeof(sigset_t)==8
> in this code, so that there is no need for an extra compat_sys_signalfd
> function.
As Stephen reported, we do need the compat in any case. Better keep all
the compat adjustments under CONFIG_COMPAT, so archs that don't need it
don't need to link to it.
> > + if ((sighand = signalfd_get_sighand(ctx, &flags)) != NULL) {
> > + if (next_signal(&ctx->tsk->pending, &ctx->sigmask) > 0 ||
> > + next_signal(&ctx->tsk->signal->shared_pending,
> > + &ctx->sigmask) > 0)
> > + events |= POLLIN;
> > + signalfd_put_sighand(ctx, sighand, &flags);
> > + } else
> > + events |= POLLIN;
> > +
> > + return events;
> > +}
>
> I never really understood the events mask, but other subsystems often
> use (POLLIN | POLLRDNORM) instead of just POLLIN. Is there a reason
> for not returning POLLRDNORM here?
I don't think those fds will have to deal with the concept of bands and
priorities. I believe POLLIN is fine here.
> > +static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
> > + siginfo_t const *kinfo)
> > +{
> > + long err;
> > +
> > + err = __clear_user(uinfo, sizeof(*uinfo));
> > +
> > + /*
> > + * If you change siginfo_t structure, please be sure
> > + * this code is fixed accordingly.
> > + */
> > + err |= __put_user(kinfo->si_signo, &uinfo->signo);
> > + err |= __put_user(kinfo->si_errno, &uinfo->err);
> > + err |= __put_user((short)kinfo->si_code, &uinfo->code);
> > + switch (kinfo->si_code & __SI_MASK) {
> > + case __SI_KILL:
> > + err |= __put_user(kinfo->si_pid, &uinfo->pid);
> > + err |= __put_user(kinfo->si_uid, &uinfo->uid);
> > + break;
> > + case __SI_TIMER:
> > + err |= __put_user(kinfo->si_tid, &uinfo->tid);
> > + err |= __put_user(kinfo->si_overrun, &uinfo->overrun);
> > + err |= __put_user(kinfo->si_ptr, &uinfo->svptr);
> > + break;
> > + case __SI_POLL:
> > + err |= __put_user(kinfo->si_band, &uinfo->band);
> > + err |= __put_user(kinfo->si_fd, &uinfo->fd);
> > + break;
> > + case __SI_FAULT:
> > + err |= __put_user(kinfo->si_addr, &uinfo->addr);
> > +#ifdef __ARCH_SI_TRAPNO
> > + err |= __put_user(kinfo->si_trapno, &uinfo->trapno);
> > +#endif
> > + break;
> > + case __SI_CHLD:
> > + err |= __put_user(kinfo->si_pid, &uinfo->pid);
> > + err |= __put_user(kinfo->si_uid, &uinfo->uid);
> > + err |= __put_user(kinfo->si_status, &uinfo->status);
> > + err |= __put_user(kinfo->si_utime, &uinfo->utime);
> > + err |= __put_user(kinfo->si_stime, &uinfo->stime);
> > + break;
> > + case __SI_RT: /* This is not generated by the kernel as of now. */
> > + case __SI_MESGQ: /* But this is */
> > + err |= __put_user(kinfo->si_pid, &uinfo->pid);
> > + err |= __put_user(kinfo->si_uid, &uinfo->uid);
> > + err |= __put_user(kinfo->si_ptr, &uinfo->svptr);
> > + break;
> > + default: /* this is just in case for now ... */
> > + err |= __put_user(kinfo->si_pid, &uinfo->pid);
> > + err |= __put_user(kinfo->si_uid, &uinfo->uid);
> > + break;
> > + }
> > +
> > + return err ? -EFAULT: sizeof(*uinfo);
> > +}
>
> Doing it this way looks rather inefficient to me. I think it's
> better to just prepare the signalfd_siginfo on the stack and
> do a single copy_to_user.
bah, __put_user is basically a move, so I don't think that efficency would
be that different (assuming that it'd matter in this case). The only thing
many __put_user do, is increase the exception table sizes.
> Also, what's the reasoning behind defining a new structure
> instead of just returning siginfo_t? Sure siginfo_t is ugly
> but it is a well-defined structure and users already deal
> with the problems it causes.
Compat on sys_read() would be insane ;)
>
> > +static void __exit signalfd_exit(void)
> > +{
> > + kmem_cache_destroy(signalfd_ctx_cachep);
> > +}
> > +
> > +module_init(signalfd_init);
> > +module_exit(signalfd_exit);
> > +
> > +MODULE_LICENSE("GPL");
>
> Since this file defines a syscall, it can't be a module, so why bother
> with this?
Agreed, remove exit function and using fs_initcall.
>
> > +
> > +struct signalfd_siginfo {
> > + __u32 signo;
> > + __s32 err;
> > + __s32 code;
> > + __u32 pid;
> > + __u32 uid;
> > + __s32 fd;
> > + __u32 tid;
> > + __u32 band;
> > + __u32 overrun;
> > + __u32 trapno;
> > + __s32 status;
> > + __s32 svint;
> > + __u64 svptr;
> > + __u64 utime;
> > + __u64 stime;
> > + __u64 addr;
> > +};
> > +
>
> Since you define the structure using __u32 etc types, I assume
> you mean it to be included from libc or other user space, right?
> In this case it needs to be listed in include/linux/Kbuild for
> make headers_install to work.
Ack!
>
> > +void signalfd_deliver(struct task_struct *tsk, int sig);
> > +
> > +/*
> > + * No need to fall inside signalfd_deliver() if no signal listeners are
> > available. + */
> > +static inline void signalfd_notify(struct task_struct *tsk, int sig)
> > +{
> > + if (unlikely(!list_empty(&tsk->sighand->sfdlist)))
> > + signalfd_deliver(tsk, sig);
> > +}
> > +
> > +static inline void signalfd_detach_locked(struct task_struct *tsk)
> > +{
> > + if (unlikely(!list_empty(&tsk->sighand->sfdlist)))
> > + signalfd_deliver(tsk, -1);
> > +}
> > +
> > +static inline void signalfd_detach(struct task_struct *tsk)
> > +{
> > + struct sighand_struct *sighand = tsk->sighand;
> > +
> > + if (unlikely(!list_empty(&sighand->sfdlist))) {
> > + spin_lock_irq(&sighand->siglock);
> > + signalfd_deliver(tsk, -1);
> > + spin_unlock_irq(&sighand->siglock);
> > + }
> > +}
> > +
>
> And all of these need to be surrounded by #ifdef __KERNEL__ so
> they don't bleed out to the user space visible parts.
Ack!
- Davide
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists