On Wednesday 18 November 2009, Andi Kleen wrote:
> > Doing all these out-of-line hidden over here in a separate file just
> > seems crazy,
> > and I'm having trouble wondering why ppl consider it a better idea at all.
>
> The separate file is on the way out, the modern way is to use
> ->compat_ioc
> Doing all these out-of-line hidden over here in a separate file just
> seems crazy,
> and I'm having trouble wondering why ppl consider it a better idea at all.
The separate file is on the way out, the modern way is to use
->compat_ioctl in the driver itself. Near all drivers except
for a few e
On Fri, Oct 30, 2009 at 8:13 PM, Arnd Bergmann wrote:
> On Friday 30 October 2009, Dave Airlie wrote:
>> Btw when I mentioned ioctls I meant more than radeon, all the KMS
>> ioctls in the common drm_crtc.c file suffer from this problem as well.
>>
>> Hence why I still believe either my drm specifi
On Friday 30 October 2009, Dave Airlie wrote:
> Btw when I mentioned ioctls I meant more than radeon, all the KMS
> ioctls in the common drm_crtc.c file suffer from this problem as well.
>
> Hence why I still believe either my drm specific inline or something
> more generic (granted I can see why
On Wed, Oct 28, 2009 at 5:53 PM, David Miller wrote:
> From: David Miller
> Date: Tue, 27 Oct 2009 23:04:50 -0700 (PDT)
>
>> From: Dave Airlie
>> Date: Wed, 28 Oct 2009 05:42:27 + (GMT)
>>
>>> I'll add this to my TODO for before the next merge window as its
>>> definitely more than I can man
On Wed, Oct 28, 2009 at 10:41:57PM -0700, David Miller wrote:
> From: Arnd Bergmann
> Date: Wed, 28 Oct 2009 16:40:18 +0100
>
> > I'm pretty sure it was ok when we started adding the compat_ioctl
> > handlers years ago. I think most people just ignored these for
> > the majority of drivers that c
On Wednesday 28 October 2009, David Miller wrote:
> > The ioctl argument actually needs a compat_ptr() conversion as well.
> > For the s390 case, we can't do that in common code, because some
> > ioctl methods put a 32 bit integer into the argument. Not sure if we
> > want to fix that everywhere, t
On Thursday 29 October 2009, David Miller wrote:
> Arnd, even compat_sys_ioctl() itself has constructs like:
>
> case FS_IOC_RESVSP:
> case FS_IOC_RESVSP64:
> error = ioctl_preallocate(filp, (void __user *)arg);
> goto out_fput;
Right. This one is p
On Wednesday 28 October 2009, David Miller wrote:
> -/* The two 64-bit arches where alignof(u64)==4 in 32-bit code */
> #if defined (CONFIG_X86_64) || defined(CONFIG_IA64)
> typedef struct drm_radeon_setparam32 {
> int param;
> u64 value;
> } __attribute__((packed)) drm_radeon_se
On Wed, 28 Oct 2009, Andi Kleen wrote:
> > I'm just amazed that compat_ioctl should be required for all new code.
> >
> > DrNick on irc suggested just doing:
> > if (is_compat_task()) ptr &= 0x;
>
> Such hacks often have problems on BE.
And then some platforms (i.e. MIPS) requi
On Wednesday 28 October 2009, Maciej W. Rozycki wrote:
>
> On Wed, 28 Oct 2009, Andi Kleen wrote:
>
> > > I'm just amazed that compat_ioctl should be required for all new code.
> > >
> > > DrNick on irc suggested just doing:
> > > if (is_compat_task()) ptr &= 0x;
> >
> > Such ha
From: Heiko Carstens
Date: Thu, 29 Oct 2009 09:34:16 +0100
> Subject: [PATCH] fs: add missing compat_ptr handling for FS_IOC_RESVSP ioctl
>
> From: Heiko Carstens
>
> For FS_IOC_RESVSP and FS_IOC_RESVSP64 compat_sys_ioctl() uses its
> arg argument as a pointer to userspace. However it is missi
From: Arnd Bergmann
Date: Wed, 28 Oct 2009 16:40:18 +0100
> I'm pretty sure it was ok when we started adding the compat_ioctl
> handlers years ago. I think most people just ignored these for
> the majority of drivers that can't possibly run on s390. Even
> on s390, gcc will always do the right th
From: Arnd Bergmann
Date: Wed, 28 Oct 2009 13:13:32 +0100
> The ioctl argument actually needs a compat_ptr() conversion as well.
> For the s390 case, we can't do that in common code, because some
> ioctl methods put a 32 bit integer into the argument. Not sure if we
> want to fix that everywhere,
From: Arnd Bergmann
Date: Wed, 28 Oct 2009 13:13:32 +0100
> IMHO a better way to handle the radeon specific ioctls would be to
> avoid the compat_alloc_user_space and just define the common
> function taking a kernel pointer, with two implementations of the
> copy operation:
Agreed, that would b
On Wednesday 28 October 2009, Andi Kleen wrote:
> > > However some architectures need special operations on compat pointers
> > > (s390 iirc), but if you don't support those it might be reasonable
> > > to not support that.
> >
> > s390 has to sign extend all 32-bit compat process pointers when
>
From: Andi Kleen
Date: Wed, 28 Oct 2009 09:19:09 +0100
> On Wed, Oct 28, 2009 at 01:11:41AM -0700, David Miller wrote:
>> From: Andi Kleen
>> Date: Wed, 28 Oct 2009 08:59:08 +0100
>>
>> >> }
>> >> - chunk_array_ptr = (uint64_t *)(unsigned long)(cs->chunks);
>> >> +#ifdef CONFIG_COMPAT
>> >> +
On Wed, Oct 28, 2009 at 01:11:41AM -0700, David Miller wrote:
> From: Andi Kleen
> Date: Wed, 28 Oct 2009 08:59:08 +0100
>
> >>}
> >> - chunk_array_ptr = (uint64_t *)(unsigned long)(cs->chunks);
> >> +#ifdef CONFIG_COMPAT
> >> + if (is_compat_task())
> >
> > Are the COMPAT ifdefs really ne
From: Andi Kleen
Date: Wed, 28 Oct 2009 08:59:08 +0100
>> }
>> -chunk_array_ptr = (uint64_t *)(unsigned long)(cs->chunks);
>> +#ifdef CONFIG_COMPAT
>> +if (is_compat_task())
>
> Are the COMPAT ifdefs really needed? The compiler should optimize that
> away anyways on non compat aware
> }
> - chunk_array_ptr = (uint64_t *)(unsigned long)(cs->chunks);
> +#ifdef CONFIG_COMPAT
> + if (is_compat_task())
Are the COMPAT ifdefs really needed? The compiler should optimize that
away anyways on non compat aware architectures, shouldn't it?
-Andi
-
From: David Miller
Date: Tue, 27 Oct 2009 23:04:50 -0700 (PDT)
> From: Dave Airlie
> Date: Wed, 28 Oct 2009 05:42:27 + (GMT)
>
>> I'll add this to my TODO for before the next merge window as its
>> definitely more than I can manage now.
>
> I'll do it.
Ok, everything was straightforward e
From: Dave Airlie
Date: Wed, 28 Oct 2009 05:42:27 + (GMT)
> I'll add this to my TODO for before the next merge window as its
> definitely more than I can manage now.
I'll do it.
--
Come build with us! The BlackBerry
> Please don't do this.
>
> This is exactly what I feared people would do when is_compat_task()
> was added. is_compat_task() is for situations where there is
> otherwise no other way to handle the compat situation properly.
>
> It's not that much work for you to hook up the compat ioctls prope
From: Andi Kleen
Date: Wed, 28 Oct 2009 05:36:11 +0100
> On Tue, Oct 27, 2009 at 08:37:09PM -0700, David Miller wrote:
>> On sparc64, in order to make debugging easier, we trap any time
>> the kernel does a userspace access to a compat task and any
>> of the upper 32-bits are non-zero.
>
> Inter
From: Dave Airlie
Date: Wed, 28 Oct 2009 03:54:34 + (GMT)
>>
>> > we already opencoded this (probably before it was macroisied or we just
>> > pasted it), so the radeon one is buggy, I should just go and compat_* all
>> > of these then and we should be all happy?
>>
>> It should be, it's
On Tue, Oct 27, 2009 at 08:37:09PM -0700, David Miller wrote:
> On sparc64, in order to make debugging easier, we trap any time
> the kernel does a userspace access to a compat task and any
> of the upper 32-bits are non-zero.
Interesting. That definitely means Dave needs a special path.
> > Howe
>
> > we already opencoded this (probably before it was macroisied or we just
> > pasted it), so the radeon one is buggy, I should just go and compat_* all
> > of these then and we should be all happy?
>
> It should be, it's only working because:
>
> 1) A malicious userland hasn't put garbage
On Tue, Oct 27, 2009 at 08:45:30PM -0700, David Miller wrote:
> From: Dave Airlie
> Date: Wed, 28 Oct 2009 03:43:07 + (GMT)
>
> > we already opencoded this (probably before it was macroisied or we just
> > pasted it), so the radeon one is buggy, I should just go and compat_* all
> > of thes
From: Dave Airlie
Date: Wed, 28 Oct 2009 03:43:07 + (GMT)
> we already opencoded this (probably before it was macroisied or we just
> pasted it), so the radeon one is buggy, I should just go and compat_* all
> of these then and we should be all happy?
It should be, it's only working becaus
>
> > DrNick on irc suggested just doing:
> > if (is_compat_task()) ptr &= 0x;
> >
> > Is there a one liner I can just do in the actual ioctls instead of
> > adding 20 compat
> > ones?
>
> Just do the right thing and pass all userland compat pointers
> through the correct compat_
From: Andi Kleen
Date: Wed, 28 Oct 2009 04:34:55 +0100
> On Wed, Oct 28, 2009 at 01:28:10PM +1000, Dave Airlie wrote:
>> Well this was what I was trying to gather, so maybe I just need to write
>> something up to state that compat_ioctl is always required for new ioctls
>> that pass pointers or 6
From: Dave Airlie
Date: Wed, 28 Oct 2009 13:28:10 +1000
> On Wed, Oct 28, 2009 at 1:19 PM, Andi Kleen wrote:
>> On Wed, Oct 28, 2009 at 01:05:08PM +1000, Dave Airlie wrote:
>>> We've designed that into a/c also, we pad all 64-bit values to 64-bit
>>> alignment on all the
>>> ioctls we've added t
From: Dave Airlie
Date: Wed, 28 Oct 2009 13:05:08 +1000
> DrNick on irc suggested just doing:
> if (is_compat_task()) ptr &= 0x;
>
> Is there a one liner I can just do in the actual ioctls instead of
> adding 20 compat
> ones?
Just do the right thing and pass all userland compat
From: Andi Kleen
Date: Wed, 28 Oct 2009 03:53:17 +0100
> Dave Airlie writes:
>
>> Now I thought cool I don't need to worry about compat ioctl hackery I can
>> run 32 on 64 bit apps fine and it'll all just work.
>>
>> Now Dave Miller points out that I'm obivously deluded and we really need
>> to
On Wed, Oct 28, 2009 at 01:28:10PM +1000, Dave Airlie wrote:
> You mean its impossible to design a 32/64-bit safe ioctl no matter what?
Not impossible, but there's always a rate of mistakes.
> and we should live with having compat ioctls? This isn't something that was
> very
> clearly stated or
On Wed, Oct 28, 2009 at 1:19 PM, Andi Kleen wrote:
> On Wed, Oct 28, 2009 at 01:05:08PM +1000, Dave Airlie wrote:
>> We've designed that into a/c also, we pad all 64-bit values to 64-bit
>> alignment on all the
>> ioctls we've added to the drm in the past couple of years. Just because of
>> this p
On Wed, Oct 28, 2009 at 01:05:08PM +1000, Dave Airlie wrote:
> We've designed that into a/c also, we pad all 64-bit values to 64-bit
> alignment on all the
> ioctls we've added to the drm in the past couple of years. Just because of
> this particular insanity.
That's actually not needed, just use
Dave Airlie writes:
> They used uint64_t to represent userspace pointers and userspace
> casted into those and the kernel casts back out and passes it to copy_*_user
uint64_t is actually dangerous due to different alignment on x86-32 vs 64,
better use compat_u64/s64
> Now I thought cool I don't
On Wed, Oct 28, 2009 at 12:53 PM, Andi Kleen wrote:
> Dave Airlie writes:
>
>> They used uint64_t to represent userspace pointers and userspace
>> casted into those and the kernel casts back out and passes it to copy_*_user
>
> uint64_t is actually dangerous due to different alignment on x86-32 v
On Wed, Oct 28, 2009 at 12:25 PM, David Miller wrote:
> From: Dave Airlie
> Date: Wed, 28 Oct 2009 11:22:18 +1000
>
>> Is there really no way to avoid compat ioctls? was I delusional in
>> thinking there was?
>
> If you use pointers in your interfaces in any way, no.
>
> And for this drm_radeon_i
From: Dave Airlie
Date: Wed, 28 Oct 2009 11:22:18 +1000
> Is there really no way to avoid compat ioctls? was I delusional in
> thinking there was?
If you use pointers in your interfaces in any way, no.
And for this drm_radeon_info thing the pointer is "pointless",
you're just returning 32-bit v
So for drm KMS we wrote a bunch of ioctls that were 64-bit clean in theory.
They used uint64_t to represent userspace pointers and userspace
casted into those and the kernel casts back out and passes it to copy_*_user
Now I thought cool I don't need to worry about compat ioctl hackery I can
run 3
42 matches
Mail list logo
