From: Arnd Bergmann <arnd@arndb.de>
To: y2038@lists.linaro.org
Cc: John Stultz <john.stultz@linaro.org>,
        Bamvor Zhang Jian <bamvor.zhangjian@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        lkml <linux-kernel@vger.kernel.org>
Subject: Re: [Y2038] [RFC PATCH v2 3/4] ppdev: add compat ioctl
Date: Wed, 08 Jul 2015 23:28:04 +0200
Message-ID: <26603653.9xWxRUDZUD@wuerfel>
User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; )
In-Reply-To: <CALAqxLUSVEbGicEiAjO=bEcMw4L=djjGLQTfEni6S8skk+H=Og@mail.gmail.com>
References: <1435587807-10008-1-git-send-email-bamvor.zhangjian@linaro.org> <1435587807-10008-4-git-send-email-bamvor.zhangjian@linaro.org> <CALAqxLUSVEbGicEiAjO=bEcMw4L=djjGLQTfEni6S8skk+H=Og@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2327
Lines: 54

On Wednesday 08 July 2015 13:17:18 John Stultz wrote:
> On Mon, Jun 29, 2015 at 7:23 AM, Bamvor Zhang Jian
> <bamvor.zhangjian@linaro.org> wrote:
> > Add compat ioctl in ppdev in order to solve the y2038 issue in
> > later patch.
> > This patch simply add pp_do_ioctl to compat_ioctl, because I found
> > that all the ioctl access the arg as a pointer.
> >
> > Signed-off-by: Bamvor Zhang Jian <bamvor.zhangjian@linaro.org>

I just saw this mail fly by when you replied, but I guess it would
have been better to reply when the original mail came.

The description above makes no sense: The problem for compat ioctl
is not whether the argument is a pointer or not, but rather what
data structure it points to. In this case, we already know that 
it is /not/ compatible between 32-bit and 64-bit user space, because
at least two commands need special handling for the timespec
argument, which gets added in patch 4 of the series.

This means patches 3 and 4 have to be swapped in order to allow
bisection and not introduce a bug when only this one gets applied
but patch 4 is missing.

Moreover, all other ioctl commands that are handled in pp_ioctl
need to be checked regarding what their arguments are, including
data structures pointed to by the arguments (recursively, if there
are again pointers in those structures).

> >         unsigned int minor = iminor(inode);
> > @@ -744,6 +750,9 @@ static const struct file_operations pp_fops = {
> >         .write          = pp_write,
> >         .poll           = pp_poll,
> >         .unlocked_ioctl = pp_ioctl,
> > +#ifdef CONFIG_COMPAT
> > +       .compat_ioctl   = pp_compat_ioctl,
> > +#endif

The #ifdef here is not necessary, but will cause a warning on kernels 
that do not define CONFIG_COMPAT, in particular all 32-bit ones.

> Does adding this patch w/o the following patch break 32bit apps using
> this on 64bit kernels?

Without the patch, those apps will all get -EINVAL from the ioctl
handler. With the patch, the kernel actually performs the ioctl
that was requested, but that may use the wrong data structure.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/