Re: [patch 2/13] signal/timer/event fds v6 - signalfd core ...

[Arnd Bergmann <arnd@arndb.de>]

On Friday 16 March 2007 01:22:15 Davide Libenzi wrote:

> +
> +static struct sighand_struct *signalfd_get_sighand(struct signalfd_ctx
> *ctx, +						   unsigned long *flags);
> +static void signalfd_put_sighand(struct signalfd_ctx *ctx,
> +				 struct sighand_struct *sighand,
> +				 unsigned long *flags);
> +static void signalfd_cleanup(struct signalfd_ctx *ctx);
> +static int signalfd_close(struct inode *inode, struct file *file);
> +static unsigned int signalfd_poll(struct file *file, poll_table *wait);
> +static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo,
> +			     siginfo_t const *kinfo);
> +static ssize_t signalfd_read(struct file *file, char __user *buf, size_t
> count, +			     loff_t *ppos);
> +

see my comment about forward declarations in the previous mail

> +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.

> +	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?

> +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.

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.

> +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?

> +
> +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.

> +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.

	Arnd <><