All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Rothwell <sfr@canb.auug.org.au>
To: Davide Libenzi <davidel@xmailserver.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	David Woodhouse <dwmw2@infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-arch@vger.kernel.org
Subject: Re: [patch] (2nd try)  add epoll compat code to kernel/compat.c ...
Date: Tue, 13 Feb 2007 15:35:25 +1100	[thread overview]
Message-ID: <20070213153525.c1440cff.sfr@canb.auug.org.au> (raw)
In-Reply-To: <Pine.LNX.4.64.0702111615340.24568@alien.or.mcafeemobile.com>

[-- Attachment #1: Type: text/plain, Size: 5048 bytes --]

Hi Davide,

[Linux-arch readers can skip to the last few paragraphs ...]

Just a couple of nits to start:

On Sun, 11 Feb 2007 16:24:30 -0800 (PST) Davide Libenzi <davidel@xmailserver.org> wrote:
>
> diff -Nru linux-2.6.20/fs/eventpoll.c linux-2.6.20.mod/fs/eventpoll.c
> --- linux-2.6.20/fs/eventpoll.c	2007-02-04 10:44:54.000000000 -0800
> +++ linux-2.6.20.mod/fs/eventpoll.c	2007-02-11 15:26:07.000000000 -0800

The changes to this file are just white space.  Don't include it with
this patch (if at all).

> diff -Nru linux-2.6.20/include/linux/compat.h linux-2.6.20.mod/include/linux/compat.h
> --- linux-2.6.20/include/linux/compat.h	2007-02-09 16:14:20.000000000 -0800
> +++ linux-2.6.20.mod/include/linux/compat.h	2007-02-11 16:11:37.000000000 -0800
> @@ -234,5 +234,35 @@
>  		compat_ulong_t maxnode, const compat_ulong_t __user *old_nodes,
>  		const compat_ulong_t __user *new_nodes);
>
> +/*
> + * epoll (fs/eventpoll.c) compat bits follow ...
> + */
> +struct epoll_event;
> +
> +struct compat_epoll_event {
> +	u32 events;
> +	u32 data[2];
> +};
> +
> +asmlinkage long compat_sys_epoll_ctl(int epfd, int op, int fd,
> +				     struct compat_epoll_event __user *event);
> +asmlinkage long compat_sys_epoll_wait(int epfd, struct compat_epoll_event __user *events,
> +				      int maxevents, int timeout);
> +/*
> + * Architectures that does not need "struct epoll_event" translation
                         ^^^^
should be "do"

> + * should wire compat_sys_epoll_pwait. The ones that needs "struct epoll_event"
                                                        ^^^^^
should be "need"

Sorry for the grammar lesson.

> + * translation, should wire compat_sys_epoll_pwait2. An architecture needs
> + * "struct epoll_event" translation is alignof(u64) in 32 bits mode is 4, and
                                       ^^                    ^^^^
should be "if" and we normally say "32 bit mode"

> + * alignof(u64) in 64 bits mode is 8.

"64 bit mode"

> +asmlinkage long compat_sys_epoll_pwait2(int epfd, struct compat_epoll_event __user *events,

Try to stay less than 80 columns wide.

> diff -Nru linux-2.6.20/kernel/compat.c linux-2.6.20.mod/kernel/compat.c
> --- linux-2.6.20/kernel/compat.c	2007-02-04 10:44:54.000000000 -0800
> +++ linux-2.6.20.mod/kernel/compat.c	2007-02-11 16:11:01.000000000 -0800
> +asmlinkage long compat_sys_epoll_ctl(int epfd, int op, int fd,
> +				     struct compat_epoll_event __user *event)
> +{
> +	long err = 0;
> +	struct compat_epoll_event user;
> +	struct epoll_event __user *kernel = NULL;
> +	union {
> +		u64 q;
> +		u32 d[2];
> +	} mux;
> +
> +	if (event) {
> +		if (copy_from_user(&user, event, sizeof(user)))
> +			return -EFAULT;
> +		kernel = compat_alloc_user_space(sizeof(struct epoll_event));
> +		err |= __put_user(user.events, &kernel->events);
> +		mux.d[0] = user.data[0];
> +		mux.d[1] = user.data[1];
> +		err |= __put_user(mux.q, &kernel->data);

A better way here might be to have each 64 bit architecture define
compat_epoll_event in its asm/compat.h and then you can just use:

	if (copy_from_user(&user, event, sizeof(user)))
		return -EFAULT;
	kernel = compat_alloc_user_space(sizeof(struct epoll_event));
	err |= __put_user(user.events, &kernel->events);
	err |= __put_user(user.data, &kernel->data);

And you shouldn't need the compat routine if
offsetof(struct compat_epoll_event, data) == offsetof(struct epoll_event, data).

> +asmlinkage long compat_sys_epoll_wait(int epfd, struct compat_epoll_event __user *events,
> +				      int maxevents, int timeout)
> +{
> +	long i, ret, err = 0;
> +	struct epoll_event __user *kbuf;
> +	struct epoll_event ev;
> +	union {
> +		u64 q;
> +		u32 d[2];
> +	} mux;
> +
> +	if (maxevents <= 0 || maxevents > (INT_MAX / sizeof(struct epoll_event)))
> +		return -EINVAL;
> +	kbuf = compat_alloc_user_space(sizeof(struct epoll_event) * maxevents);
> +	ret = sys_epoll_wait(epfd, kbuf, maxevents, timeout);
> +	for (i = 0; i < ret; i++) {
> +		err |= __get_user(ev.events, &kbuf[i].events);
> +		err |= __get_user(ev.data, &kbuf[i].data);
> +		err |= put_user(ev.events, &events->events);
> +		mux.q = ev.data;
> +		err |= put_user(mux.d[0], &events->data[0]);
> +		err |= put_user(mux.d[1], &events->data[1]);
> +		events++;
> +	}

Similarly here.

OK, I have thought about this some more and I *think* the only
architecture that needs compat_sys_epoll_ctl or compat_sys_epoll_wait is
ia64 where the 64 bit version of struct epoll_event is different from the
32 bit version.  On x86_64, the struct is explictly packed (so it is the
same as the 32 bit version) and on all the other 64 bit architectures the
alignment of the u64 is the same as the equivalent 32 bit version.

Since ia64 already has its own version of these two, we only have to
worry about epoll_pwait and then the struct epoll_event is only a problem
for ia64.

Am I right?  (I have cc'd linux-arch for guidance.)

(sorry for the long early bit of this mail)
--
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

  reply	other threads:[~2007-02-13  4:35 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-12  0:24 Davide Libenzi
2007-02-13  4:35 ` Stephen Rothwell [this message]
2007-02-13  7:26   ` Davide Libenzi
2007-02-13 10:11     ` Stephen Rothwell
2007-02-13 14:43   ` James Bottomley
2007-02-13 23:17     ` Stephen Rothwell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070213153525.c1440cff.sfr@canb.auug.org.au \
    --to=sfr@canb.auug.org.au \
    --cc=akpm@linux-foundation.org \
    --cc=davidel@xmailserver.org \
    --cc=dwmw2@infradead.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [patch] (2nd try)  add epoll compat code to kernel/compat.c ...' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.