linux-next: build failure after merge of the aio tree

linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

Hi Benjamin,

After merging the aio tree, today's linux-next build (arm
multi_v7_defconfig) failed like this:

fs/built-in.o: In function `aio_thread_op_foo_at':
file.c:(.text+0x43808): undefined reference to `__get_user_bad'
file.c:(.text+0x43838): undefined reference to `__get_user_bad'

Caused by commit

  150a0b4905f1 ("aio: add support for async openat()")

I have used the aio tree from next-20160111 for today.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Tue, Jan 12, 2016 at 04:40:34PM +1100, Stephen Rothwell wrote:
> Hi Benjamin,
> 
> After merging the aio tree, today's linux-next build (arm
> multi_v7_defconfig) failed like this:
> 
> fs/built-in.o: In function `aio_thread_op_foo_at':
> file.c:(.text+0x43808): undefined reference to `__get_user_bad'
> file.c:(.text+0x43838): undefined reference to `__get_user_bad'

This is very strange.  It seems to imply that __get_user() doesn't 
handle 64 bit values, which is completely broken behaviour on the 
architecture's part if true.  Can any ARM folks comment on the right 
fix here?

		-ben

> Caused by commit
> 
>   150a0b4905f1 ("aio: add support for async openat()")
> 
> I have used the aio tree from next-20160111 for today.
> 
> -- 
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au

-- 
"Thought is the essence of where you are now."

Re: linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

Hi Benjamin,

On Tue, 12 Jan 2016 16:40:34 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> After merging the aio tree, today's linux-next build (arm
> multi_v7_defconfig) failed like this:
> 
> fs/built-in.o: In function `aio_thread_op_foo_at':
> file.c:(.text+0x43808): undefined reference to `__get_user_bad'
> file.c:(.text+0x43838): undefined reference to `__get_user_bad'
> 
> Caused by commit
> 
>   150a0b4905f1 ("aio: add support for async openat()")
> 
> I have used the aio tree from next-20160111 for today.

I am still getting this error.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

Re: linux-next: build failure after merge of the aio tree

[Christoph Hellwig]

On Tue, Jan 12, 2016 at 04:40:34PM +1100, Stephen Rothwell wrote:
> Hi Benjamin,
> 
> After merging the aio tree, today's linux-next build (arm
> multi_v7_defconfig) failed like this:
> 
> fs/built-in.o: In function `aio_thread_op_foo_at':
> file.c:(.text+0x43808): undefined reference to `__get_user_bad'
> file.c:(.text+0x43838): undefined reference to `__get_user_bad'
> 
> Caused by commit
> 
>   150a0b4905f1 ("aio: add support for async openat()")

How did that code end up in linux-next anyway?

Re: linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

Hi Christoph,

On Thu, 14 Jan 2016 23:39:44 -0800 Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jan 12, 2016 at 04:40:34PM +1100, Stephen Rothwell wrote:
> > Hi Benjamin,
> > 
> > After merging the aio tree, today's linux-next build (arm
> > multi_v7_defconfig) failed like this:
> > 
> > fs/built-in.o: In function `aio_thread_op_foo_at':
> > file.c:(.text+0x43808): undefined reference to `__get_user_bad'
> > file.c:(.text+0x43838): undefined reference to `__get_user_bad'
> > 
> > Caused by commit
> > 
> >   150a0b4905f1 ("aio: add support for async openat()")  
> 
> How did that code end up in linux-next anyway?

Via the aio tree (git://git.kvack.org/~bcrl/aio-next.git#master) added
in July 2013 at Ben's request.  The code was added to the aio tree in
Jan 12 (my time), but has never been in a published linux-next tree due
to the above build problem (I back out to the previous days version of
the aio tree).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

Re: linux-next: build failure after merge of the aio tree

[Christoph Hellwig]

On Fri, Jan 15, 2016 at 08:23:16PM +1100, Stephen Rothwell wrote:
> Via the aio tree (git://git.kvack.org/~bcrl/aio-next.git#master) added
> in July 2013 at Ben's request.  The code was added to the aio tree in
> Jan 12 (my time), but has never been in a published linux-next tree due
> to the above build problem (I back out to the previous days version of
> the aio tree).

Well, it's code Ben posted a few days ago, which to say it mildly is
rather controversial.  It's cetainly not 4.5 material.

> 
> -- 
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au
---end quoted text---

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Fri, Jan 15, 2016 at 01:25:31AM -0800, Christoph Hellwig wrote:
> On Fri, Jan 15, 2016 at 08:23:16PM +1100, Stephen Rothwell wrote:
> > Via the aio tree (git://git.kvack.org/~bcrl/aio-next.git#master) added
> > in July 2013 at Ben's request.  The code was added to the aio tree in
> > Jan 12 (my time), but has never been in a published linux-next tree due
> > to the above build problem (I back out to the previous days version of
> > the aio tree).
> 
> Well, it's code Ben posted a few days ago, which to say it mildly is
> rather controversial.  It's cetainly not 4.5 material.

It still needs the exposure.

As for the build failure, it's a bug in the arch __get_user() implementation 
that needs to be fixed.  __get_user() should really be able to handle 64 bit 
types.

		-ben

> > -- 
> > Cheers,
> > Stephen Rothwell                    sfr@canb.auug.org.au
> ---end quoted text---

-- 
"Thought is the essence of where you are now."

Re: linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

Hi Ben,

On Fri, 15 Jan 2016 10:18:21 -0500 Benjamin LaHaise <bcrl@kvack.org> wrote:
>
> On Fri, Jan 15, 2016 at 01:25:31AM -0800, Christoph Hellwig wrote:
> > On Fri, Jan 15, 2016 at 08:23:16PM +1100, Stephen Rothwell wrote:  
> > > Via the aio tree (git://git.kvack.org/~bcrl/aio-next.git#master) added
> > > in July 2013 at Ben's request.  The code was added to the aio tree in
> > > Jan 12 (my time), but has never been in a published linux-next tree due
> > > to the above build problem (I back out to the previous days version of
> > > the aio tree).  
> > 
> > Well, it's code Ben posted a few days ago, which to say it mildly is
> > rather controversial.  It's cetainly not 4.5 material.  
> 
> It still needs the exposure.

If it is not destined for v4.5, then it should not (yet) be in
linux-next.  It should wait until after v4.5-rc1 is released (the merge
window closes).  I would also argue that if the functionality itself is
still under active review (and I haven't competely followed the
discussion so I don't know where that is up to, but Christoph, at
least, seems not completely convinced), then it should also not yet be
in linux-next.

> As for the build failure, it's a bug in the arch __get_user() implementation 
> that needs to be fixed.  __get_user() should really be able to handle 64 bit 
> types.

Yeah, it is a bit weird.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

Re: linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

Hi Benjamin,

On Tue, 12 Jan 2016 11:38:35 -0500 Benjamin LaHaise <bcrl@kvack.org> wrote:
>
> On Tue, Jan 12, 2016 at 04:40:34PM +1100, Stephen Rothwell wrote:
> > 
> > After merging the aio tree, today's linux-next build (arm
> > multi_v7_defconfig) failed like this:
> > 
> > fs/built-in.o: In function `aio_thread_op_foo_at':
> > file.c:(.text+0x43808): undefined reference to `__get_user_bad'
> > file.c:(.text+0x43838): undefined reference to `__get_user_bad'  
> 
> This is very strange.  It seems to imply that __get_user() doesn't 
> handle 64 bit values, which is completely broken behaviour on the 
> architecture's part if true.  Can any ARM folks comment on the right 
> fix here?

Well, probably only if you cc them :-)

Indeed, __get_user on arm does not handle 64 bit objects, where as
get_user does ...

Background: new aio code is adding __get_user() calls referencing 64
bit quantities (__u64 and __s64).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

Re: linux-next: build failure after merge of the aio tree

[Russell King - ARM Linux]

On Wed, Jan 27, 2016 at 01:40:24PM +1100, Stephen Rothwell wrote:
> Hi Benjamin,
> 
> On Tue, 12 Jan 2016 11:38:35 -0500 Benjamin LaHaise <bcrl@kvack.org> wrote:
> >
> > On Tue, Jan 12, 2016 at 04:40:34PM +1100, Stephen Rothwell wrote:
> > > 
> > > After merging the aio tree, today's linux-next build (arm
> > > multi_v7_defconfig) failed like this:
> > > 
> > > fs/built-in.o: In function `aio_thread_op_foo_at':
> > > file.c:(.text+0x43808): undefined reference to `__get_user_bad'
> > > file.c:(.text+0x43838): undefined reference to `__get_user_bad'  
> > 
> > This is very strange.  It seems to imply that __get_user() doesn't 
> > handle 64 bit values, which is completely broken behaviour on the 
> > architecture's part if true.  Can any ARM folks comment on the right 
> > fix here?
> 
> Well, probably only if you cc them :-)
> 
> Indeed, __get_user on arm does not handle 64 bit objects, where as
> get_user does ...

I believe adding 64-bit support to __get_user() is going to be a really
painful exercise.  It took long enough to make get_user() accept it
without creating any new warnings, and then bug fix it for big endian.
__get_user() has the added complexity that it's inline assembler, and
we can't play the same games that we used in __get_user().

It's not a simple case of:

 #define __get_user_err(x, ptr, err)					\
 do {									\
 	unsigned long __gu_addr = (unsigned long)(ptr); 		\
-	unsigned long __gu_val; 					\
+	unsigned long long __gu_val; 					\
 	unsigned int __ua_flags;					\
 	__chk_user_ptr(ptr);						\
 	might_fault();							\
 	__ua_flags = uaccess_save_and_enable(); 			\
 	switch (sizeof(*(ptr))) {					\
 	case 1: __get_user_asm_byte(__gu_val, __gu_addr, err);	break;	\
 	case 2: __get_user_asm_half(__gu_val, __gu_addr, err);	break;	\
 	case 4: __get_user_asm_word(__gu_val, __gu_addr, err);	break;	\
+	case 8: __get_user_asm_dword(__gu_val, __gu_addr, err); break;	\
 	default: (__gu_val) = __get_user_bad(); 			\
 	}								\
 	uaccess_restore(__ua_flags);					\
 	(x) = (__typeof__(*(ptr)))__gu_val;				\
 } while (0)

as, while that works for integer 'x', it doesn't work when 'x' is a
pointer type, as the cast on the last line creates a GCC warning
about casting to a different size.

You can't move the cast into the switch to eliminate that, because
GCC still warns even though the code is unreachable.  Same problem
exists if you move the variable declaration inside the switch.

> Background: new aio code is adding __get_user() calls referencing 64
> bit quantities (__u64 and __s64).

There's lots more architectures which do not support 64-bit get_user()
_or_ __get_user(): avr32, blackfin, metag for example, and m68k which
has this interesting thing "/* case 8: disabled because gcc-4.1 has a
broken typeof \" in its *get_user() implementation.

Even x86-32 doesn't support it:

#define __get_user(x, ptr)                                              \
        __get_user_nocheck((x), (ptr), sizeof(*(ptr)))

#define __get_user_nocheck(x, ptr, size)                                \
({                                                                      \
        int __gu_err;                                                   \
        unsigned long __gu_val;                                         \
        __uaccess_begin();                                              \
        __get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT);    \
        __uaccess_end();                                                \
        (x) = (__force __typeof__(*(ptr)))__gu_val;                     \
        __builtin_expect(__gu_err, 0);                                  \
})

#define __get_user_size(x, ptr, size, retval, errret)                   \
do {                                                                    \
        retval = 0;                                                     \
        __chk_user_ptr(ptr);                                            \
        switch (size) {                                                 \
...
        case 8:                                                         \
                __get_user_asm_u64(x, ptr, retval, errret);             \
                break;                                                  \
        default:                                                        \
                (x) = __get_user_bad();                                 \
        }                                                               \
} while (0)

#ifdef CONFIG_X86_32
#define __get_user_asm_u64(x, ptr, retval, errret)      (x) = __get_user_bad()
#define __get_user_asm_ex_u64(x, ptr)                   (x) = __get_user_bad()

Note that __get_user_nocheck() above always uses a 32-bit integer for
__gu_val, so fixing x86-32 will not be a case of just providing a
better __get_user_asm_u64() - it's the same problem that I have on ARM.

The problem for 32-bit architectures is being able to cope sanely with:
* different endians on the same arch
* gcc warning about casting to different sizes, even when the code is
  unreachable
* preserving proper and correct typechecking in get_user() and friends

So, I suspect if AIO were to run a test build on x86-32, they would find
the same errors.

Anyone working on this needs to ensure that any implementation builds
without warning on all of these:

int test_8(unsigned char *v, unsigned char *p)
{
        return __get_user(*v, p);
}

int test_8_constp(unsigned char *v, const unsigned char *p)
{
        return __get_user(*v, p);
}

int test_8_volatilep(unsigned char *v, volatile unsigned char *p)
{
        return __get_user(*v, p);
}

int test_16(unsigned short *v, unsigned short *p)
{
        return __get_user(*v, p);
}

int test_16_constp(unsigned short *v, const unsigned short *p)
{
        return __get_user(*v, p);
}

int test_32(unsigned int *v, unsigned int *p)
{
        return __get_user(*v, p);
}

int test_32_constp(unsigned int *v, const unsigned int *p)
{
        return __get_user(*v, p);
}

int test_64(unsigned long long *v, unsigned long long *p)
{
        return __get_user(*v, p);
}

int test_64_constp(unsigned long long *v, const unsigned long long *p)
{
        return __get_user(*v, p);
}

int test_ptr(unsigned int **v, unsigned int **p)
{
        return __get_user(*v, p);
}

int test_const(unsigned int *v, const unsigned int *p)
{
        return __get_user(*v, p);
}

int test_64_narrow(unsigned long *v, unsigned long long *p)
{
        return __get_user(*v, p);
}

int test_32_wide(unsigned long long *v, unsigned long *p)
{
        return __get_user(*v, p);
}

except for this which must warn:

int test_wrong(char **v, const char **p)
{
        return __get_user(*v, p);
}

which comes from my test set for when get_user() was being worked on.

So, in summary, it seems that the 32-bit architectures I've looked at
so far do not support 64-bit __get_user(), even the popular ones.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: linux-next: build failure after merge of the aio tree

[Geert Uytterhoeven]

On Fri, Jan 29, 2016 at 12:30 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>> Background: new aio code is adding __get_user() calls referencing 64
>> bit quantities (__u64 and __s64).
>
> There's lots more architectures which do not support 64-bit get_user()
> _or_ __get_user(): avr32, blackfin, metag for example, and m68k which
> has this interesting thing "/* case 8: disabled because gcc-4.1 has a
> broken typeof \" in its *get_user() implementation.

And if you enable it again, you get lots of "warning: cast to pointer from
integer of different size", like you mentioned.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

Hi Benjamin,

On Fri, 29 Jan 2016 13:03:39 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> On Fri, Jan 29, 2016 at 12:30 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >> Background: new aio code is adding __get_user() calls referencing 64
> >> bit quantities (__u64 and __s64).  
> >
> > There's lots more architectures which do not support 64-bit get_user()
> > _or_ __get_user(): avr32, blackfin, metag for example, and m68k which
> > has this interesting thing "/* case 8: disabled because gcc-4.1 has a
> > broken typeof \" in its *get_user() implementation.  
> 
> And if you enable it again, you get lots of "warning: cast to pointer from
> integer of different size", like you mentioned.

Any thoughts?  I am still using the version of tha aio tree from
next-20160111.

-- 
Cheers,
Stephen Rothwell

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Thu, Feb 04, 2016 at 01:19:59PM +1100, Stephen Rothwell wrote:
> Hi Benjamin,
> 
> On Fri, 29 Jan 2016 13:03:39 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > On Fri, Jan 29, 2016 at 12:30 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > >> Background: new aio code is adding __get_user() calls referencing 64
> > >> bit quantities (__u64 and __s64).  
> > >
> > > There's lots more architectures which do not support 64-bit get_user()
> > > _or_ __get_user(): avr32, blackfin, metag for example, and m68k which
> > > has this interesting thing "/* case 8: disabled because gcc-4.1 has a
> > > broken typeof \" in its *get_user() implementation.  
> > 
> > And if you enable it again, you get lots of "warning: cast to pointer from
> > integer of different size", like you mentioned.
> 
> Any thoughts?  I am still using the version of tha aio tree from
> next-20160111.

I am still convinced that this is an architecture issue.  Given that 64 bit 
values work in the *get_user implementations on other architectures, I see 
no reason there should need to be a workaround for this in common code.

		-ben

> -- 
> Cheers,
> Stephen Rothwell

-- 
"Thought is the essence of where you are now."

Re: linux-next: build failure after merge of the aio tree

[Russell King - ARM Linux]

On Thu, Feb 04, 2016 at 08:41:42AM -0500, Benjamin LaHaise wrote:
> On Thu, Feb 04, 2016 at 01:19:59PM +1100, Stephen Rothwell wrote:
> > Hi Benjamin,
> > 
> > On Fri, 29 Jan 2016 13:03:39 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >
> > > On Fri, Jan 29, 2016 at 12:30 PM, Russell King - ARM Linux
> > > <linux@arm.linux.org.uk> wrote:
> > > >> Background: new aio code is adding __get_user() calls referencing 64
> > > >> bit quantities (__u64 and __s64).  
> > > >
> > > > There's lots more architectures which do not support 64-bit get_user()
> > > > _or_ __get_user(): avr32, blackfin, metag for example, and m68k which
> > > > has this interesting thing "/* case 8: disabled because gcc-4.1 has a
> > > > broken typeof \" in its *get_user() implementation.  
> > > 
> > > And if you enable it again, you get lots of "warning: cast to pointer from
> > > integer of different size", like you mentioned.
> > 
> > Any thoughts?  I am still using the version of tha aio tree from
> > next-20160111.
> 
> I am still convinced that this is an architecture issue.  Given that 64 bit 
> values work in the *get_user implementations on other architectures, I see 
> no reason there should need to be a workaround for this in common code.

So you're happy to break x86-32 then...

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Thu, Feb 04, 2016 at 01:50:56PM +0000, Russell King - ARM Linux wrote:
> > I am still convinced that this is an architecture issue.  Given that 64 bit 
> > values work in the *get_user implementations on other architectures, I see 
> > no reason there should need to be a workaround for this in common code.
> 
> So you're happy to break x86-32 then...

x86-32 works fine.

		-ben
-- 
"Thought is the essence of where you are now."

Re: linux-next: build failure after merge of the aio tree

[Russell King - ARM Linux]

On Thu, Feb 04, 2016 at 09:08:22AM -0500, Benjamin LaHaise wrote:
> On Thu, Feb 04, 2016 at 01:50:56PM +0000, Russell King - ARM Linux wrote:
> > > I am still convinced that this is an architecture issue.  Given that 64 bit 
> > > values work in the *get_user implementations on other architectures, I see 
> > > no reason there should need to be a workaround for this in common code.
> > 
> > So you're happy to break x86-32 then...
> 
> x86-32 works fine.

Let me repeat the quote from my previous email:

#define __get_user(x, ptr)                                              \
        __get_user_nocheck((x), (ptr), sizeof(*(ptr)))

#define __get_user_nocheck(x, ptr, size)                                \
({                                                                      \
        int __gu_err;                                                   \
        unsigned long __gu_val;                                         \
        __uaccess_begin();                                              \
        __get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT);    \
        __uaccess_end();                                                \
        (x) = (__force __typeof__(*(ptr)))__gu_val;                     \
        __builtin_expect(__gu_err, 0);                                  \
})

#define __get_user_size(x, ptr, size, retval, errret)                   \
do {                                                                    \
        retval = 0;                                                     \
        __chk_user_ptr(ptr);                                            \
        switch (size) {                                                 \
        case 1:                                                         \
                __get_user_asm(x, ptr, retval, "b", "b", "=q", errret); \
                break;                                                  \
        case 2:                                                         \
                __get_user_asm(x, ptr, retval, "w", "w", "=r", errret); \
                break;                                                  \
        case 4:                                                         \
                __get_user_asm(x, ptr, retval, "l", "k", "=r", errret); \
                break;                                                  \
        case 8:                                                         \
                __get_user_asm_u64(x, ptr, retval, errret);             \
                break;                                                  \
        default:                                                        \
                (x) = __get_user_bad();                                 \
        }                                                               \
} while (0)

#ifdef CONFIG_X86_32
#define __get_user_asm_u64(x, ptr, retval, errret)      (x) = __get_user_bad()
#define __get_user_asm_ex_u64(x, ptr)                   (x) = __get_user_bad()
#else
#define __get_user_asm_u64(x, ptr, retval, errret) \
         __get_user_asm(x, ptr, retval, "q", "", "=r", errret)
#define __get_user_asm_ex_u64(x, ptr) \
         __get_user_asm_ex(x, ptr, "q", "", "=r")
#endif

Hence, __get_user() on x86-32 with a 64-bit quantity results in
__get_user_bad() being called, which is an undefined function.
Only if you build with x86-64 support enabled (iow, CONFIG_X86_32 not
defined) then you get the 64-bit __get_user() support.

Given this, I fail to see how x86-32 can possibly work.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Thu, Feb 04, 2016 at 02:12:53PM +0000, Russell King - ARM Linux wrote:
> Hence, __get_user() on x86-32 with a 64-bit quantity results in
> __get_user_bad() being called, which is an undefined function.
> Only if you build with x86-64 support enabled (iow, CONFIG_X86_32 not
> defined) then you get the 64-bit __get_user() support.
> 
> Given this, I fail to see how x86-32 can possibly work.

You're right; mea culpa.  It compiles without warning on x86-32, but it 
does not link.  I still think this is broken archtecture stupidity since 
put_user() works for 64 bit data types.

		-ben
-- 
"Thought is the essence of where you are now."

Re: linux-next: build failure after merge of the aio tree

[Russell King - ARM Linux]

On Thu, Feb 04, 2016 at 09:32:04AM -0500, Benjamin LaHaise wrote:
> On Thu, Feb 04, 2016 at 02:12:53PM +0000, Russell King - ARM Linux wrote:
> > Hence, __get_user() on x86-32 with a 64-bit quantity results in
> > __get_user_bad() being called, which is an undefined function.
> > Only if you build with x86-64 support enabled (iow, CONFIG_X86_32 not
> > defined) then you get the 64-bit __get_user() support.
> > 
> > Given this, I fail to see how x86-32 can possibly work.
> 
> You're right; mea culpa.  It compiles without warning on x86-32, but it 
> does not link.  I still think this is broken archtecture stupidity since 
> put_user() works for 64 bit data types.

Indeed, and you'll find that several other architectures besides ARM and
x86-32 have exactly the same problem - as I listed in my message from a
few days ago.

Okay, so now I get to set you a challenge, since you're the one wanting
64-bit __get_user(): try implementing it on x86-32 :)

Also in my previous message from a few days ago I provided a set of
functions which test out the implementation.  Here they are... again.

All these should not produce any warnings, and should produce correct
code - especially the narrowing/widening tests:

int test_8(unsigned char *v, unsigned char *p)
{ return __get_user(*v, p); }
int test_8_constp(unsigned char *v, const unsigned char *p)
{ return __get_user(*v, p); }
int test_8_volatilep(unsigned char *v, volatile unsigned char *p)
{ return __get_user(*v, p); }
int test_16(unsigned short *v, unsigned short *p)
{ return __get_user(*v, p); }
int test_16_constp(unsigned short *v, const unsigned short *p)
{ return __get_user(*v, p); }
int test_32(unsigned int *v, unsigned int *p)
{ return __get_user(*v, p); }
int test_32_constp(unsigned int *v, const unsigned int *p)
{ return __get_user(*v, p); }
int test_64(unsigned long long *v, unsigned long long *p)
{ return __get_user(*v, p); }
int test_64_constp(unsigned long long *v, const unsigned long long *p)
{ return __get_user(*v, p); }
int test_ptr(unsigned int **v, unsigned int **p)
{ return __get_user(*v, p); }
int test_const(unsigned int *v, const unsigned int *p)
{ return __get_user(*v, p); }
int test_64_narrow(unsigned long *v, unsigned long long *p)
{ return __get_user(*v, p); }
int test_32_wide(unsigned long long *v, unsigned long *p)
{ return __get_user(*v, p); }

However, this one should warn:

int test_wrong(char **v, const char **p)
{ return __get_user(*v, p); }

Good luck (I think you'll need lots of it to get a working solution)! :)

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Thu, Feb 04, 2016 at 02:39:07PM +0000, Russell King - ARM Linux wrote:
> However, this one should warn:
> 
> int test_wrong(char **v, const char **p)
> { return __get_user(*v, p); }
> 
> Good luck (I think you'll need lots of it to get a working solution)! :)

This works with your test cases on x86-32.  Note that it's only compile + 
link tested at present.

		-ben
-- 
"Thought is the essence of where you are now."

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 09b1b0a..d8834c2 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -326,7 +326,21 @@ do {									\
 } while (0)
 
 #ifdef CONFIG_X86_32
-#define __get_user_asm_u64(x, ptr, retval, errret)	(x) = __get_user_bad()
+#define __get_user_asm_u64(x, addr, err, errret)			\
+	asm volatile(ASM_STAC "\n"					\
+		     "1:	movl %2,%%eax\n"			\
+		     "		movl %3,%%edx\n"			\
+		     "2: " ASM_CLAC "\n"				\
+		     ".section .fixup,\"ax\"\n"				\
+		     "3:	mov %4,%0\n"				\
+		     "	xorl %%eax,%%eax\n"				\
+		     "	xorl %%edx,%%edx\n"				\
+		     "	jmp 2b\n"					\
+		     ".previous\n"					\
+		     _ASM_EXTABLE(1b, 3b)				\
+		     : "=r" (err), "=A"(x)				\
+		     : "m" (__m(addr)), "m" __m(((u32 *)addr) + 1), "i" (errret), "0" (err))
+
 #define __get_user_asm_ex_u64(x, ptr)			(x) = __get_user_bad()
 #else
 #define __get_user_asm_u64(x, ptr, retval, errret) \

Re: linux-next: build failure after merge of the aio tree

[Russell King - ARM Linux]

On Thu, Feb 04, 2016 at 11:01:01AM -0500, Benjamin LaHaise wrote:
> On Thu, Feb 04, 2016 at 02:39:07PM +0000, Russell King - ARM Linux wrote:
> > However, this one should warn:
> > 
> > int test_wrong(char **v, const char **p)
> > { return __get_user(*v, p); }
> > 
> > Good luck (I think you'll need lots of it to get a working solution)! :)
> 
> This works with your test cases on x86-32.  Note that it's only compile + 
> link tested at present.

That's the easy bit!

The problem you're going to run into is here:

#define __get_user_nocheck(x, ptr, size)                                \
({                                                                      \
        int __gu_err;                                                   \
        unsigned long __gu_val;                                         \
        __uaccess_begin();                                              \
        __get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT);    \
        __uaccess_end();                                                \
        (x) = (__force __typeof__(*(ptr)))__gu_val;                     \

__gu_val will be 32-bit, even when you're wanting a 64-bit quantity.
That's where the fun and games start...

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Thu, Feb 04, 2016 at 04:17:42PM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 04, 2016 at 11:01:01AM -0500, Benjamin LaHaise wrote:
> > On Thu, Feb 04, 2016 at 02:39:07PM +0000, Russell King - ARM Linux wrote:
> > > However, this one should warn:
> > > 
> > > int test_wrong(char **v, const char **p)
> > > { return __get_user(*v, p); }
> > > 
> > > Good luck (I think you'll need lots of it to get a working solution)! :)
> > 
> > This works with your test cases on x86-32.  Note that it's only compile + 
> > link tested at present.
> 
> That's the easy bit!
> 
> The problem you're going to run into is here:
> 
> #define __get_user_nocheck(x, ptr, size)                                \
> ({                                                                      \
>         int __gu_err;                                                   \
>         unsigned long __gu_val;                                         \
>         __uaccess_begin();                                              \
>         __get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT);    \
>         __uaccess_end();                                                \
>         (x) = (__force __typeof__(*(ptr)))__gu_val;                     \
> 
> __gu_val will be 32-bit, even when you're wanting a 64-bit quantity.
> That's where the fun and games start...

Ugh.  You're making me install a 32 bit distro.....!

		-ben

> -- 
> RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

-- 
"Thought is the essence of where you are now."

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Thu, Feb 04, 2016 at 04:17:42PM +0000, Russell King - ARM Linux wrote:
> That's the easy bit!
> 
> The problem you're going to run into is here:
> 
> #define __get_user_nocheck(x, ptr, size)                                \
> ({                                                                      \
>         int __gu_err;                                                   \
>         unsigned long __gu_val;                                         \
>         __uaccess_begin();                                              \
>         __get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT);    \
>         __uaccess_end();                                                \
>         (x) = (__force __typeof__(*(ptr)))__gu_val;                     \
> 
> __gu_val will be 32-bit, even when you're wanting a 64-bit quantity.
> That's where the fun and games start...

You're right -- it's quite non-trivial.  How evil would it be to make a 
separate __get_user64() macro?

		-ben
-- 
"Thought is the essence of where you are now."

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Thu, Feb 04, 2016 at 04:17:42PM +0000, Russell King - ARM Linux wrote:
> __gu_val will be 32-bit, even when you're wanting a 64-bit quantity.
> That's where the fun and games start...

Okay, I figured out how to do it: instead of using a 64 bit unsigned long 
long for __gu_val, use an array of 2 unsigned longs.  See the patch below 
which I verified boots, passes your tests and doesn't truncate 64 bit 
values.  Comments?  A simple test module to verify things is located at 
http://www.kvack.org/~bcrl/test_module2.c .

		-ben
-- 
"Thought is the essence of where you are now."


diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 09b1b0a..53244ae 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -326,7 +326,22 @@ do {									\
 } while (0)
 
 #ifdef CONFIG_X86_32
-#define __get_user_asm_u64(x, ptr, retval, errret)	(x) = __get_user_bad()
+#define __get_user_asm_u64(x, addr, err, errret)			\
+	asm volatile(ASM_STAC "\n"					\
+		     "1:	movl %2,%%eax\n"			\
+		     "2:	movl %3,%%edx\n"			\
+		     "3: " ASM_CLAC "\n"				\
+		     ".section .fixup,\"ax\"\n"				\
+		     "4:	mov %4,%0\n"				\
+		     "	xorl %%eax,%%eax\n"				\
+		     "	xorl %%edx,%%edx\n"				\
+		     "	jmp 3b\n"					\
+		     ".previous\n"					\
+		     _ASM_EXTABLE(1b, 4b)				\
+		     _ASM_EXTABLE(2b, 4b)				\
+		     : "=r" (err), "=A"(x)				\
+		     : "m" (__m(addr)), "m" __m(((u32 *)addr) + 1), "i" (errret), "0" (err))
+
 #define __get_user_asm_ex_u64(x, ptr)			(x) = __get_user_bad()
 #else
 #define __get_user_asm_u64(x, ptr, retval, errret) \
@@ -407,9 +422,16 @@ do {									\
 #define __get_user_nocheck(x, ptr, size)				\
 ({									\
 	int __gu_err;							\
-	unsigned long __gu_val;						\
-	__get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT);	\
-	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
+	if (size == 8) {						\
+		unsigned long __gu_val[2];				\
+		__gu_err = 0;						\
+		__get_user_asm_u64(__gu_val, ptr, __gu_err, -EFAULT);	\
+		(x) = *(__force __typeof__((ptr)))__gu_val;		\
+	} else {							\
+		unsigned long __gu_val;					\
+		__get_user_size(__gu_val, (ptr), (size), __gu_err, -EFAULT); \
+		(x) = (__force __typeof__(*(ptr)))__gu_val;		\
+	}								\
 	__builtin_expect(__gu_err, 0);					\
 })
 

Re: linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

Hi Ben,

On Sat, 16 Jan 2016 09:55:15 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> On Fri, 15 Jan 2016 10:18:21 -0500 Benjamin LaHaise <bcrl@kvack.org> wrote:
> >
> > On Fri, Jan 15, 2016 at 01:25:31AM -0800, Christoph Hellwig wrote:  
> > > On Fri, Jan 15, 2016 at 08:23:16PM +1100, Stephen Rothwell wrote:    
> > > > Via the aio tree (git://git.kvack.org/~bcrl/aio-next.git#master) added
> > > > in July 2013 at Ben's request.  The code was added to the aio tree in
> > > > Jan 12 (my time), but has never been in a published linux-next tree due
> > > > to the above build problem (I back out to the previous days version of
> > > > the aio tree).    
> > > 
> > > Well, it's code Ben posted a few days ago, which to say it mildly is
> > > rather controversial.  It's cetainly not 4.5 material.    
> > 
> > It still needs the exposure.  
> 
> If it is not destined for v4.5, then it should not (yet) be in
> linux-next.  It should wait until after v4.5-rc1 is released (the merge
> window closes).  I would also argue that if the functionality itself is
> still under active review (and I haven't competely followed the
> discussion so I don't know where that is up to, but Christoph, at
> least, seems not completely convinced), then it should also not yet be
> in linux-next.

OK, so at this point (just to get rid of the build failure I have done this:

I have reset the aio tree head to commit

  b47275df9e1c ("aio: add support for aio poll via aio thread helper")

and then cherry-picked the following commits on top:

  fb2e69217129 ("aio: Fix compile error due to unexpected use of cmpxchg()")
  0964acffc614 ("aio: revert addition of io_send_sig() in generic_write_checks")

> > As for the build failure, it's a bug in the arch __get_user() implementation 
> > that needs to be fixed.  __get_user() should really be able to handle 64 bit 
> > types.  
> 
> Yeah, it is a bit weird.

Well, you need to negotiate that with the affected architectures.

-- 
Cheers,
Stephen Rothwell

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Mon, Mar 14, 2016 at 03:49:15PM +1100, Stephen Rothwell wrote:
> Hi Ben,

...
> OK, so at this point (just to get rid of the build failure I have done this:
...
> Well, you need to negotiate that with the affected architectures.

I put in a patch to use get_user() for now since the 32 bit architectures 
don't seem to have any plans for fixing this issue in a predictable 
timeframe.  There shouldn't be any build failures now -- I've checked ia64, 
i386 and x86_64.  The merge conflict looks trivial, so I won't touch 
those pieces for the time being.

		-ben
-- 
"Thought is the essence of where you are now."

Re: linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

Hi Ben,

On Mon, 14 Mar 2016 13:08:07 -0400 Benjamin LaHaise <bcrl@kvack.org> wrote:
>
> I put in a patch to use get_user() for now since the 32 bit architectures 
> don't seem to have any plans for fixing this issue in a predictable 
> timeframe.  There shouldn't be any build failures now -- I've checked ia64, 
> i386 and x86_64.  The merge conflict looks trivial, so I won't touch 
> those pieces for the time being.

OK, thanks for the heads up.

-- 
Cheers,
Stephen Rothwell

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Wed, Mar 16, 2016 at 02:59:38PM +0100, Arnd Bergmann wrote:
> On Wednesday 16 March 2016 13:12:36 Andy Shevchenko wrote:
> > 
> > > I've also sent a patch that fixes the link error on ARM and that should
> > > work on all other architectures too.
> > 
> > In case of avr32 signalfd_read() fails. Does your patch help with it as well?
> > 
> > P.S. Bisecting shows same culprit: 150a0b4905f1 ("aio: add support for
> > async openat()")
> 
> I don't know. What is the symptom on avr32? My patch only removes the
> get_user() instances on 64-bit values and replaces them with a
> single copy_from_user() call.

Which is the wrong fix.  Arch code should be able to handle 64 bit values 
in all the get/put_user() variants.  We use 64 bit variables all over the 
place in interfaces to userspace.

		-ben

Re: linux-next: build failure after merge of the aio tree

[Arnd Bergmann]

On Wednesday 16 March 2016 13:12:36 Andy Shevchenko wrote:
> 
> > I've also sent a patch that fixes the link error on ARM and that should
> > work on all other architectures too.
> 
> In case of avr32 signalfd_read() fails. Does your patch help with it as well?
> 
> P.S. Bisecting shows same culprit: 150a0b4905f1 ("aio: add support for
> async openat()")

I don't know. What is the symptom on avr32? My patch only removes the
get_user() instances on 64-bit values and replaces them with a
single copy_from_user() call.

	Arnd

Re: linux-next: build failure after merge of the aio tree

[Andy Shevchenko]

On Wed, Mar 16, 2016 at 12:02 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 15 March 2016 12:22:28 Benjamin LaHaise wrote:
>> On Tue, Mar 15, 2016 at 04:19:02PM +0000, Sudip Mukherjee wrote:
>> > On Tue, Mar 15, 2016 at 05:46:34PM +1100, Stephen Rothwell wrote:
>> > > Hi Benjamin,
>> > >
>> > > After merging the aio tree, today's linux-next build (powerpc
>> > > ppc44x_defconfig) failed like this:
>> > >
>> > > fs/built-in.o: In function `aio_thread_op_foo_at':
>> > > aio.c:(.text+0x4dab4): undefined reference to `__get_user_bad'
>> > > aio.c:(.text+0x4daec): undefined reference to `__get_user_bad'
>> > >
>> > > Caused by commit
>> > >
>> > >   150a0b4905f1 ("aio: add support for async openat()")
>> > >
>> > > despite commit
>> > >
>> > >   d2f7a973e11e ("aio: don't use __get_user() for 64 bit values")
>> > >

>> I've removed everything from the aio-next.git tree for now.  Will revisit
>> after the merge window.

I think it is the best solution right now.

> I've also sent a patch that fixes the link error on ARM and that should
> work on all other architectures too.

In case of avr32 signalfd_read() fails. Does your patch help with it as well?

P.S. Bisecting shows same culprit: 150a0b4905f1 ("aio: add support for
async openat()")

-- 
With Best Regards,
Andy Shevchenko

Re: linux-next: build failure after merge of the aio tree

[Arnd Bergmann]

On Tuesday 15 March 2016 12:22:28 Benjamin LaHaise wrote:
> On Tue, Mar 15, 2016 at 04:19:02PM +0000, Sudip Mukherjee wrote:
> > On Tue, Mar 15, 2016 at 05:46:34PM +1100, Stephen Rothwell wrote:
> > > Hi Benjamin,
> > > 
> > > After merging the aio tree, today's linux-next build (powerpc
> > > ppc44x_defconfig) failed like this:
> > > 
> > > fs/built-in.o: In function `aio_thread_op_foo_at':
> > > aio.c:(.text+0x4dab4): undefined reference to `__get_user_bad'
> > > aio.c:(.text+0x4daec): undefined reference to `__get_user_bad'
> > > 
> > > Caused by commit
> > > 
> > >   150a0b4905f1 ("aio: add support for async openat()")
> > > 
> > > despite commit
> > > 
> > >   d2f7a973e11e ("aio: don't use __get_user() for 64 bit values")
> > > 
> > > This is due to a bug in the powerpc __get_user_check() macro (the return
> > > value is defined to be "unsigned long" which is only 32 bits on a 32
> > > bit platform).
> > 
> > m68k allmodconfig and all defs of m32r fails while building next-20160315.
> > 
> > regards
> > sudip
> 
> I've removed everything from the aio-next.git tree for now.  Will revisit 
> after the merge window.
> 

I've also sent a patch that fixes the link error on ARM and that should
work on all other architectures too.

	Arnd

Re: linux-next: build failure after merge of the aio tree

[Arnd Bergmann]

On Tuesday 15 March 2016 16:38:51 Andy Shevchenko wrote:
> On Tue, Mar 15, 2016 at 8:46 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi Benjamin,
> >
> > After merging the aio tree, today's linux-next build (powerpc
> > ppc44x_defconfig) failed like this:
> >
> > fs/built-in.o: In function `aio_thread_op_foo_at':
> > aio.c:(.text+0x4dab4): undefined reference to `__get_user_bad'
> > aio.c:(.text+0x4daec): undefined reference to `__get_user_bad'
> 
> avr32 seems affected as well and the below solution is not suitable
> (should be much more verbose in asm, I guess).
> 

ARM as well, at least for ARMv7-M.

	Arnd

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Tue, Mar 15, 2016 at 04:19:02PM +0000, Sudip Mukherjee wrote:
> On Tue, Mar 15, 2016 at 05:46:34PM +1100, Stephen Rothwell wrote:
> > Hi Benjamin,
> > 
> > After merging the aio tree, today's linux-next build (powerpc
> > ppc44x_defconfig) failed like this:
> > 
> > fs/built-in.o: In function `aio_thread_op_foo_at':
> > aio.c:(.text+0x4dab4): undefined reference to `__get_user_bad'
> > aio.c:(.text+0x4daec): undefined reference to `__get_user_bad'
> > 
> > Caused by commit
> > 
> >   150a0b4905f1 ("aio: add support for async openat()")
> > 
> > despite commit
> > 
> >   d2f7a973e11e ("aio: don't use __get_user() for 64 bit values")
> > 
> > This is due to a bug in the powerpc __get_user_check() macro (the return
> > value is defined to be "unsigned long" which is only 32 bits on a 32
> > bit platform).
> 
> m68k allmodconfig and all defs of m32r fails while building next-20160315.
> 
> regards
> sudip

I've removed everything from the aio-next.git tree for now.  Will revisit 
after the merge window.

		-ben
-- 
"Thought is the essence of where you are now."

Re: linux-next: build failure after merge of the aio tree

[Sudip Mukherjee]

On Tue, Mar 15, 2016 at 05:46:34PM +1100, Stephen Rothwell wrote:
> Hi Benjamin,
> 
> After merging the aio tree, today's linux-next build (powerpc
> ppc44x_defconfig) failed like this:
> 
> fs/built-in.o: In function `aio_thread_op_foo_at':
> aio.c:(.text+0x4dab4): undefined reference to `__get_user_bad'
> aio.c:(.text+0x4daec): undefined reference to `__get_user_bad'
> 
> Caused by commit
> 
>   150a0b4905f1 ("aio: add support for async openat()")
> 
> despite commit
> 
>   d2f7a973e11e ("aio: don't use __get_user() for 64 bit values")
> 
> This is due to a bug in the powerpc __get_user_check() macro (the return
> value is defined to be "unsigned long" which is only 32 bits on a 32
> bit platform).

m68k allmodconfig and all defs of m32r fails while building next-20160315.

regards
sudip

Re: linux-next: build failure after merge of the aio tree

[Andy Shevchenko]

On Tue, Mar 15, 2016 at 8:46 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi Benjamin,
>
> After merging the aio tree, today's linux-next build (powerpc
> ppc44x_defconfig) failed like this:
>
> fs/built-in.o: In function `aio_thread_op_foo_at':
> aio.c:(.text+0x4dab4): undefined reference to `__get_user_bad'
> aio.c:(.text+0x4daec): undefined reference to `__get_user_bad'

avr32 seems affected as well and the below solution is not suitable
(should be much more verbose in asm, I guess).

>
> Caused by commit
>
>   150a0b4905f1 ("aio: add support for async openat()")
>
> despite commit
>
>   d2f7a973e11e ("aio: don't use __get_user() for 64 bit values")
>
> This is due to a bug in the powerpc __get_user_check() macro (the return
> value is defined to be "unsigned long" which is only 32 bits on a 32
> bit platform).
>
> I applied the patch below which seems to help (Michael, what do you
> think?), but given Al's and Christoph's reactions, I am inclined to
> remove the aio tree from tomorrow and maybe it can be revisited after
> the merge window.
>
> From: Stephen Rothwell <sfr@canb.auug.org.au>
> Date: Tue, 15 Mar 2016 16:36:06 +1100
> Subject: [PATCH] powerpc: fix get_user for 64 bit typs on 32 bit platforms
>
> solution borrowed from the x86 uaccess.h
>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
>  arch/powerpc/include/asm/uaccess.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index b7c20f0b8fbe..52262b2f37fc 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -261,10 +261,13 @@ do {                                                              \
>         }                                                       \
>  } while (0)
>
> +#define __inttype(x) \
> +       __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> +
>  #define __get_user_nocheck(x, ptr, size)                       \
>  ({                                                             \
>         long __gu_err;                                          \
> -       unsigned long __gu_val;                                 \
> +       __inttype(*(ptr)) __gu_val;                             \
>         __typeof__(*(ptr)) __user *__gu_addr = (ptr);   \
>         __chk_user_ptr(ptr);                                    \
>         if (!is_kernel_addr((unsigned long)__gu_addr))          \
> @@ -277,7 +280,7 @@ do {                                                                \
>  #define __get_user_check(x, ptr, size)                                 \
>  ({                                                                     \
>         long __gu_err = -EFAULT;                                        \
> -       unsigned long  __gu_val = 0;                                    \
> +       __inttype(*(ptr)) __gu_val = 0;                         \
>         __typeof__(*(ptr)) __user *__gu_addr = (ptr);           \
>         might_fault();                                                  \
>         if (access_ok(VERIFY_READ, __gu_addr, (size)))                  \
> @@ -289,7 +292,7 @@ do {                                                                \
>  #define __get_user_nosleep(x, ptr, size)                       \
>  ({                                                             \
>         long __gu_err;                                          \
> -       unsigned long __gu_val;                                 \
> +       __inttype(*(ptr)) __gu_val;                             \
>         __typeof__(*(ptr)) __user *__gu_addr = (ptr);   \
>         __chk_user_ptr(ptr);                                    \
>         __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> --
> 2.7.0
>
> --
> Cheers,
> Stephen Rothwell



-- 
With Best Regards,
Andy Shevchenko

linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

Hi Benjamin,

After merging the aio tree, today's linux-next build (powerpc
ppc44x_defconfig) failed like this:

fs/built-in.o: In function `aio_thread_op_foo_at':
aio.c:(.text+0x4dab4): undefined reference to `__get_user_bad'
aio.c:(.text+0x4daec): undefined reference to `__get_user_bad'

Caused by commit

  150a0b4905f1 ("aio: add support for async openat()")

despite commit

  d2f7a973e11e ("aio: don't use __get_user() for 64 bit values")

This is due to a bug in the powerpc __get_user_check() macro (the return
value is defined to be "unsigned long" which is only 32 bits on a 32
bit platform).

I applied the patch below which seems to help (Michael, what do you
think?), but given Al's and Christoph's reactions, I am inclined to
remove the aio tree from tomorrow and maybe it can be revisited after
the merge window.

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Tue, 15 Mar 2016 16:36:06 +1100
Subject: [PATCH] powerpc: fix get_user for 64 bit typs on 32 bit platforms

solution borrowed from the x86 uaccess.h

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 arch/powerpc/include/asm/uaccess.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index b7c20f0b8fbe..52262b2f37fc 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -261,10 +261,13 @@ do {								\
 	}							\
 } while (0)
 
+#define __inttype(x) \
+	__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
+
 #define __get_user_nocheck(x, ptr, size)			\
 ({								\
 	long __gu_err;						\
-	unsigned long __gu_val;					\
+	__inttype(*(ptr)) __gu_val;				\
 	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
 	__chk_user_ptr(ptr);					\
 	if (!is_kernel_addr((unsigned long)__gu_addr))		\
@@ -277,7 +280,7 @@ do {								\
 #define __get_user_check(x, ptr, size)					\
 ({									\
 	long __gu_err = -EFAULT;					\
-	unsigned long  __gu_val = 0;					\
+	__inttype(*(ptr)) __gu_val = 0;				\
 	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
 	might_fault();							\
 	if (access_ok(VERIFY_READ, __gu_addr, (size)))			\
@@ -289,7 +292,7 @@ do {								\
 #define __get_user_nosleep(x, ptr, size)			\
 ({								\
 	long __gu_err;						\
-	unsigned long __gu_val;					\
+	__inttype(*(ptr)) __gu_val;				\
 	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
 	__chk_user_ptr(ptr);					\
 	__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\
-- 
2.7.0

-- 
Cheers,
Stephen Rothwell

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

On Fri, Aug 30, 2013 at 10:38:25AM -0700, Linus Torvalds wrote:
> On Fri, Aug 30, 2013 at 12:55 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > -                       aio_ring_file->f_path.dentry->d_count,
> > +                       aio_ring_file->f_path.dentry->d_lockref.count,
> 
> This is wrong. If you really want the dentry count (which I doubt, I
> don't see why this code would care _even_ just for a debug printout),
> you should use

Agreed -- it was just debugging code from initial development of the code, 
so I just ended up deleting it instead.

		-ben
-- 
"Thought is the essence of where you are now."

Re: linux-next: build failure after merge of the aio tree

[Linus Torvalds]

On Fri, Aug 30, 2013 at 12:55 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> -                       aio_ring_file->f_path.dentry->d_count,
> +                       aio_ring_file->f_path.dentry->d_lockref.count,

This is wrong. If you really want the dentry count (which I doubt, I
don't see why this code would care _even_ just for a debug printout),
you should use

   d_count(aio_ring_file->f_dentry)

instead these days. That will get the count from the right place,
regardless of any lockref issues or anything else (and f_dentry may be
the "old" way to get the dentry, but it's still supported and unlikely
to go away. No point in writing out "f_path.dentry" unless you *want*
to be aware of the fact that f_path has both a dentry and a mnt member
- but most people really don't care about the mnt information).

            Linus

Re: linux-next: build failure after merge of the aio tree

[Benjamin LaHaise]

Hi Stephen,

On Fri, Aug 30, 2013 at 05:55:09PM +1000, Stephen Rothwell wrote:
> Hi Benjamin,
> 
> After merging the aio tree, today's linux-next build (powerpc
> ppc64_defconfig) failed like this:
> 
> In file included from include/linux/kernel.h:13:0,
>                  from fs/aio.c:13:
> fs/aio.c: In function 'aio_free_ring':
> fs/aio.c:188:32: error: 'struct dentry' has no member named 'd_count'
>     aio_ring_file->f_path.dentry->d_count,
>                                 ^
> include/linux/printk.h:246:38: note: in definition of macro 'pr_debug'
>   no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)

Ah, this is unnecessary debugging code that got changed.  I've committed 
the following to my aio-next tree to just delete the code since it is not 
required.

		-ben
-- 
"Thought is the essence of where you are now."


commit 79bd1bcf1ab22ea723da7d5854a9e72a350ecbf8
Author: Benjamin LaHaise <bcrl@kvack.org>
Date:   Fri Aug 30 10:22:04 2013 -0400

    aio: remove unnecessary debugging from aio_free_ring()
    
    The commit 36bc08cc0170 ("fs/aio: Add support to aio ring pages migration")
    added some debugging code that is not required and resulted in a build error
    when 98474236f72e ("vfs: make the dentry cache use the lockref infrastructure")
    was added to the tree.  The code is not required, so just delete it.
    
    Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>

diff --git a/fs/aio.c b/fs/aio.c
index c3f005d..d0defcb 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -183,11 +183,6 @@ static void aio_free_ring(struct kioctx *ctx)
 
 	if (aio_ring_file) {
 		truncate_setsize(aio_ring_file->f_inode, 0);
-		pr_debug("pid(%d) i_nlink=%u d_count=%d d_unhashed=%d i_count=%d\n",
-			current->pid, aio_ring_file->f_inode->i_nlink,
-			aio_ring_file->f_path.dentry->d_count,
-			d_unhashed(aio_ring_file->f_path.dentry),
-			atomic_read(&aio_ring_file->f_inode->i_count));
 		fput(aio_ring_file);
 		ctx->aio_ring_file = NULL;
 	}

linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

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

Hi Benjamin,

After merging the aio tree, today's linux-next build (powerpc
ppc64_defconfig) failed like this:

In file included from include/linux/kernel.h:13:0,
                 from fs/aio.c:13:
fs/aio.c: In function 'aio_free_ring':
fs/aio.c:188:32: error: 'struct dentry' has no member named 'd_count'
    aio_ring_file->f_path.dentry->d_count,
                                ^
include/linux/printk.h:246:38: note: in definition of macro 'pr_debug'
  no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
                                      ^

Caused by commit 36bc08cc0170 ("s/aio: Add support to aio ring pages
migration") interacting with commit 98474236f72e ("vfs: make the dentry
cache use the lockref infrastructure") from Linus' tree.

I applied the following (probably suboptimal) fix and can carry it as
necessary.

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Fri, 30 Aug 2013 17:49:10 +1000
Subject: [PATCH] aio: fixup for lockref changes

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 fs/aio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/aio.c b/fs/aio.c
index 9f783e3..6d00cd9 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -185,7 +185,7 @@ static void aio_free_ring(struct kioctx *ctx)
 		truncate_setsize(aio_ring_file->f_inode, 0);
 		pr_debug("pid(%d) i_nlink=%u d_count=%d d_unhashed=%d i_count=%d\n",
 			current->pid, aio_ring_file->f_inode->i_nlink,
-			aio_ring_file->f_path.dentry->d_count,
+			aio_ring_file->f_path.dentry->d_lockref.count,
 			d_unhashed(aio_ring_file->f_path.dentry),
 			atomic_read(&aio_ring_file->f_inode->i_count));
 		fput(aio_ring_file);
-- 
1.8.4.rc3

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

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

Re: linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

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

Hi Dave,

On Wed, 21 Aug 2013 10:52:27 -0500 Dave Kleikamp <dave.kleikamp@oracle.com> wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> There's another problem after the merge. Now that buf is passed into
> aio_run_iocb(), ki_iter is no longer needed. It's not getting
> initialized and causes an oops.
> 
> Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>

OK, thanks, this will be applied to the aio tree merge today.

> - ---

BTW, your GPG clearsigning corrupts patches.  You can clearsign them
using the --not-dash-escaped option to gpg (I think).  I fixed this up.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

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

Re: linux-next: build failure after merge of the aio tree

[Dave Kleikamp]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Stephen,

There's another problem after the merge. Now that buf is passed into
aio_run_iocb(), ki_iter is no longer needed. It's not getting
initialized and causes an oops.

Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>
- ---
 fs/aio.c            | 4 ++--
 include/linux/aio.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 59b46cd..9f783e3 100644
- --- a/fs/aio.c
+++ b/fs/aio.c
@@ -1281,11 +1281,11 @@ rw_common:
 		break;
 
 	case IOCB_CMD_READ_ITER:
- -		ret = aio_read_iter(req, req->ki_iter);
+		ret = aio_read_iter(req, (void *)buf);
 		break;
 
 	case IOCB_CMD_WRITE_ITER:
- -		ret = aio_write_iter(req, req->ki_iter);
+		ret = aio_write_iter(req, (void *)buf);
 		break;
 
 	case IOCB_CMD_FDSYNC:
diff --git a/include/linux/aio.h b/include/linux/aio.h
index 4fab377..c08d3ed 100644
- --- a/include/linux/aio.h
+++ b/include/linux/aio.h
@@ -54,7 +54,6 @@ struct kiocb {
 	 * this is the underlying eventfd context to deliver events to.
 	 */
 	struct eventfd_ctx	*ki_eventfd;
- -	struct iov_iter		*ki_iter;
 };
 
 static inline bool is_sync_kiocb(struct kiocb *kiocb)
- -- 
1.8.3.4

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSFOI7AAoJEDaohF61QIxkuW4P/3XIggPvVE3ZypC2Qno9UwtB
KXCVu0A0eLNbA6tB82vCWaQ7IblazoCOAex74qpGGrSB4ReS8yJDHkqnFR1aK8i4
Jyi2EONVUzAq5ijH9amZQfFWwcXBsH6nb2uEcv/xQ2/lc1hPCl3kIy/fKYVRWfYB
7YfX5J3VQnrI28B8NEccfqtTdQlYGp7VaRbh9dKVsudjBZZW9LAMkNcWoFuJzL9l
mAtubeUSaq9F7Dp6p/8whXyh5Ue2jPKo2m87B1et9Y5RN+K45uX3hTUJcAdKY72k
p9U4KNTCFG5o5FfsxI62B3/41R7KeOSe0PvILmb822vxLBnQWgF+fTScswKDDUcP
BFLoKE0WeCgzBiIPLEHaxJZSW7wUJqYAYGrnHRM8yFjJHUMDkWjiNN/597MDbijF
5BBj+gLN2GiBxKQcjtZRTXpUZ2pThLhPYpz/REtS134wz3Hp+ftsytHISCeUUoso
Icf8qd+JmggLOugdXg0gq7LAAc7Wnm91TJ79jsXFc7HPRBKl7AflS3e5poSQoy+Y
YZfVCUWV8LAnnB/fa9JobCIajIOebhhODpd5Y1hcqd9YDrqzhF3KlwyQkAmy9e2x
kX2dvHcumSNkYzWmQnP8Yol7oDWA9XXaL0nYRKDkpy/3F9+pCeheO1bY7tUnMD9F
34UQznE6M8rRkgiRKGDe
=WSg+
-----END PGP SIGNATURE-----

linux-next: build failure after merge of the aio tree

[Stephen Rothwell]

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

Hi Benjamin,

After merging the aio tree, today's linux-next build (powerpc
ppc64_defconfig) failed like this:

fs/aio.c: In function 'aio_kernel_init_rw':
fs/aio.c:1359:6: error: 'struct kiocb' has no member named 'ki_left'
  iocb->ki_left = nr;
      ^
fs/aio.c: In function 'aio_kernel_submit':
fs/aio.c:1394:6: error: 'struct kiocb' has no member named 'ki_opcode'
  iocb->ki_opcode = op;
      ^
fs/aio.c:1395:6: error: 'struct kiocb' has no member named 'ki_buf'
  iocb->ki_buf = (char __user *)(unsigned long)ptr;
      ^
fs/aio.c:1398:2: error: too few arguments to function 'aio_run_iocb'
  ret = aio_run_iocb(iocb, 0);
  ^
fs/aio.c:1217:16: note: declared here
 static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode,
                ^

Caused by commit af9fa2024c38 ("aio: add aio_kernel_() interface") from
the aio-direct tree interacting with commit 8bc92afcf7f5 ("aio: Kill
unneeded kiocb members") from the aio tree.

I applied the following merge fix patch (and I can carry it as necessary
- thanks, Dave, for the hint patches):

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Wed, 21 Aug 2013 17:42:14 +1000
Subject: [PATCH] aio: semantic fixup

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 fs/aio.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 39fb7b0..59b46cd 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1356,7 +1356,6 @@ void aio_kernel_init_rw(struct kiocb *iocb, struct file *filp,
 			size_t nr, loff_t off)
 {
 	iocb->ki_filp = filp;
-	iocb->ki_left = nr;
 	iocb->ki_nbytes = nr;
 	iocb->ki_pos = off;
 	iocb->ki_ctx = (void *)-1;
@@ -1391,11 +1390,7 @@ int aio_kernel_submit(struct kiocb *iocb, unsigned short op, void *ptr)
 	BUG_ON(!iocb->ki_obj.complete);
 	BUG_ON(!iocb->ki_filp);
 
-	iocb->ki_opcode = op;
-	iocb->ki_buf = (char __user *)(unsigned long)ptr;
-	iocb->ki_iter = ptr;
-
-	ret = aio_run_iocb(iocb, 0);
+	ret = aio_run_iocb(iocb, op, ptr, 0);
 
 	if (ret)
 		aio_kernel_free(iocb);
-- 
1.8.4.rc3

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

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