Re: [PATCH 3/4] c/r: [signal] pending signals (private, shared)

[Oren Laadan <orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org>]

Dan Smith wrote:
> OL> +static int load_siginfo(siginfo_t *info, struct ckpt_hdr_siginfo *si)
> OL> +{
> OL> +	if (!valid_signal(si->signo))
> OL> +		return -EINVAL;
> OL> +
> OL> +	info->si_signo = si->signo;
> OL> +	info->si_errno = si->_errno;
> OL> +	info->si_code = si->code;
> OL> +
> OL> +	/* TODO: validate remaining signal fields */
> OL> +
> OL> +	switch(info->si_code & __SI_MASK) {
> OL> +	case __SI_TIMER:
> OL> +		info->si_tid = si->pid;
> OL> +		info->si_overrun = si->uid;
> OL> +		info->si_int = si->sigval_int;
> OL> +		info->si_sys_private = si->utime;
> OL> +		break;
> OL> +	case __SI_POLL:
> OL> +		info->si_band = si->pid;
> OL> +		info->si_fd = si->sigval_int;
> OL> +		break;
> OL> +	case __SI_FAULT:
> OL> +		info->si_addr = (void __user *) (unsigned long) si->sigval_ptr;
> OL> +#ifdef __ARCH_SI_TRAPNO
> OL> +		info->si_trapno = si->sigval_int;
> OL> +#endif
> OL> +		break;
> OL> +	case __SI_CHLD:
> OL> +		info->si_pid = si->pid;
> OL> +		info->si_uid = si->uid;
> OL> +		info->si_status = si->sigval_int;
> OL> +		info->si_stime = si->stime;
> OL> +		info->si_utime = si->utime;
> OL> +		break;
> OL> +	case __SI_KILL:
> OL> +	case __SI_RT:
> OL> +	case __SI_MESGQ:
> OL> +		info->si_pid = si->pid;
> OL> +		info->si_uid = si->uid;
> OL> +		info->si_ptr = (void __user *) (unsigned long) si->sigval_ptr;
> OL> +		break;
> OL> +	default:
> OL> +		return -EINVAL;
> OL> +	}
> OL> +
> OL> +	return 0;
> OL> +}
> OL> +
> 
> This seems like a perfect place to use the CKPT_COPY() macros, if
> we're going to have them.  This and the save equivalent could be
> almost identical.

Hehe... "if" ?!

Anyway, I'll see what I can do.


> OL> +static int restore_sigpending(struct ckpt_ctx *ctx, struct sigpending *pending)
> OL> +{
> OL> +	struct ckpt_hdr_sigpending *h;
> OL> +	struct ckpt_hdr_siginfo *si;
> OL> +	struct sigqueue *q;
> OL> +	int ret = 0;
> OL> +
> OL> +	h = ckpt_read_buf_type(ctx, 0, CKPT_HDR_SIGPENDING);
> OL> +	if (IS_ERR(h))
> OL> +		return PTR_ERR(h);
> OL> +
> OL> +	INIT_LIST_HEAD(&pending->list);
> OL> +	load_sigset(&pending->signal, &h->signal);
> OL> +
> OL> +	si = h->siginfo;
> OL> +	while (h->nr_pending--) {
> OL> +		q = sigqueue_alloc();
> OL> +		if (!q) {
> OL> +			ret = -ENOMEM;
> OL> +			break;
> OL> +		}
> OL> +
> OL> +		ret = load_siginfo(&q->info, si++);
> 
> I think there should be a sanity check here, no?  The checkpoint
> stream claims h->nr_pending structures in h->siginfo, but we can't
> trust that we're not going to march through memory to a segv unless we
> check that the header length matches your calculation in
> checkpoint_sigpending().
> 

I think you are right :p

Thanks,

Oren.