* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Greg KH @ 2020-10-22 12:18 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022104805.GA1503673@kroah.com>
On Thu, Oct 22, 2020 at 12:48:05PM +0200, Greg KH wrote:
> On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
> > On 22.10.20 11:32, David Laight wrote:
> > > From: David Hildenbrand
> > >> Sent: 22 October 2020 10:25
> > > ...
> > >> ... especially because I recall that clang and gcc behave slightly
> > >> differently:
> > >>
> > >> https://github.com/hjl-tools/x86-psABI/issues/2
> > >>
> > >> "Function args are different: narrow types are sign or zero extended to
> > >> 32 bits, depending on their type. clang depends on this for incoming
> > >> args, but gcc doesn't make that assumption. But both compilers do it
> > >> when calling, so gcc code can call clang code.
> > >
> > > It really is best to use 'int' (or even 'long') for all numeric
> > > arguments (and results) regardless of the domain of the value.
> > >
> > > Related, I've always worried about 'bool'....
> > >
> > >> The upper 32 bits of registers are always undefined garbage for types
> > >> smaller than 64 bits."
> > >
> > > On x86-64 the high bits are zeroed by all 32bit loads.
> >
> > Yeah, but does not help here.
> >
> >
> > My thinking: if the compiler that calls import_iovec() has garbage in
> > the upper 32 bit
> >
> > a) gcc will zero it out and not rely on it being zero.
> > b) clang will not zero it out, assuming it is zero.
> >
> > But
> >
> > a) will zero it out when calling the !inlined variant
> > b) clang will zero it out when calling the !inlined variant
> >
> > When inlining, b) strikes. We access garbage. That would mean that we
> > have calling code that's not generated by clang/gcc IIUC.
> >
> > We can test easily by changing the parameters instead of adding an "inline".
>
> Let me try that as well, as I seem to have a good reproducer, but it
> takes a while to run...
Ok, that didn't work.
And I can't seem to "fix" this by adding noinline to patches further
along in the patch series (because this commit's function is no longer
present due to later patches.)
Will keep digging...
greg k-h
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Hildenbrand @ 2020-10-22 12:42 UTC (permalink / raw)
To: Greg KH
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022121849.GA1664412@kroah.com>
On 22.10.20 14:18, Greg KH wrote:
> On Thu, Oct 22, 2020 at 12:48:05PM +0200, Greg KH wrote:
>> On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
>>> On 22.10.20 11:32, David Laight wrote:
>>>> From: David Hildenbrand
>>>>> Sent: 22 October 2020 10:25
>>>> ...
>>>>> ... especially because I recall that clang and gcc behave slightly
>>>>> differently:
>>>>>
>>>>> https://github.com/hjl-tools/x86-psABI/issues/2
>>>>>
>>>>> "Function args are different: narrow types are sign or zero extended to
>>>>> 32 bits, depending on their type. clang depends on this for incoming
>>>>> args, but gcc doesn't make that assumption. But both compilers do it
>>>>> when calling, so gcc code can call clang code.
>>>>
>>>> It really is best to use 'int' (or even 'long') for all numeric
>>>> arguments (and results) regardless of the domain of the value.
>>>>
>>>> Related, I've always worried about 'bool'....
>>>>
>>>>> The upper 32 bits of registers are always undefined garbage for types
>>>>> smaller than 64 bits."
>>>>
>>>> On x86-64 the high bits are zeroed by all 32bit loads.
>>>
>>> Yeah, but does not help here.
>>>
>>>
>>> My thinking: if the compiler that calls import_iovec() has garbage in
>>> the upper 32 bit
>>>
>>> a) gcc will zero it out and not rely on it being zero.
>>> b) clang will not zero it out, assuming it is zero.
>>>
>>> But
>>>
>>> a) will zero it out when calling the !inlined variant
>>> b) clang will zero it out when calling the !inlined variant
>>>
>>> When inlining, b) strikes. We access garbage. That would mean that we
>>> have calling code that's not generated by clang/gcc IIUC.
>>>
>>> We can test easily by changing the parameters instead of adding an "inline".
>>
>> Let me try that as well, as I seem to have a good reproducer, but it
>> takes a while to run...
>
> Ok, that didn't work.
>
> And I can't seem to "fix" this by adding noinline to patches further
> along in the patch series (because this commit's function is no longer
> present due to later patches.)
We might have the same issues with iovec_from_user() and friends now.
>
> Will keep digging...
>
> greg k-h
>
Might be worth to give this a try, just to see if it's related to
garbage in upper 32 bit and the way clang is handling it (might be a BUG
in clang, though):
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 72d88566694e..7527298c6b56 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -267,7 +267,7 @@ size_t hash_and_copy_to_iter(const void *addr,
size_t bytes, void *hashp,
struct iov_iter *i);
struct iovec *iovec_from_user(const struct iovec __user *uvector,
- unsigned long nr_segs, unsigned long fast_segs,
+ unsigned nr_segs, unsigned fast_segs,
struct iovec *fast_iov, bool compat);
ssize_t import_iovec(int type, const struct iovec __user *uvec,
unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 1635111c5bd2..58417f1916dc 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1652,7 +1652,7 @@ const void *dup_iter(struct iov_iter *new, struct
iov_iter *old, gfp_t flags)
EXPORT_SYMBOL(dup_iter);
static int copy_compat_iovec_from_user(struct iovec *iov,
- const struct iovec __user *uvec, unsigned long nr_segs)
+ const struct iovec __user *uvec, unsigned nr_segs)
{
const struct compat_iovec __user *uiov =
(const struct compat_iovec __user *)uvec;
@@ -1684,7 +1684,7 @@ static int copy_compat_iovec_from_user(struct
iovec *iov,
}
static int copy_iovec_from_user(struct iovec *iov,
- const struct iovec __user *uvec, unsigned long nr_segs)
+ const struct iovec __user *uvec, unsigned nr_segs)
{
unsigned long seg;
@@ -1699,7 +1699,7 @@ static int copy_iovec_from_user(struct iovec *iov,
}
struct iovec *iovec_from_user(const struct iovec __user *uvec,
- unsigned long nr_segs, unsigned long fast_segs,
+ unsigned nr_segs, unsigned fast_segs,
struct iovec *fast_iov, bool compat)
{
struct iovec *iov = fast_iov;
@@ -1738,7 +1738,7 @@ ssize_t __import_iovec(int type, const struct
iovec __user *uvec,
struct iov_iter *i, bool compat)
{
ssize_t total_len = 0;
- unsigned long seg;
+ unsigned seg;
struct iovec *iov;
iov = iovec_from_user(uvec, nr_segs, fast_segs, *iovp, compat);
--
Thanks,
David / dhildenb
^ permalink raw reply related
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Greg KH @ 2020-10-22 12:57 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <98d9df88-b7ef-fdfb-7d90-2fa7a9d7bab5@redhat.com>
On Thu, Oct 22, 2020 at 02:42:24PM +0200, David Hildenbrand wrote:
> On 22.10.20 14:18, Greg KH wrote:
> > On Thu, Oct 22, 2020 at 12:48:05PM +0200, Greg KH wrote:
> >> On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
> >>> On 22.10.20 11:32, David Laight wrote:
> >>>> From: David Hildenbrand
> >>>>> Sent: 22 October 2020 10:25
> >>>> ...
> >>>>> ... especially because I recall that clang and gcc behave slightly
> >>>>> differently:
> >>>>>
> >>>>> https://github.com/hjl-tools/x86-psABI/issues/2
> >>>>>
> >>>>> "Function args are different: narrow types are sign or zero extended to
> >>>>> 32 bits, depending on their type. clang depends on this for incoming
> >>>>> args, but gcc doesn't make that assumption. But both compilers do it
> >>>>> when calling, so gcc code can call clang code.
> >>>>
> >>>> It really is best to use 'int' (or even 'long') for all numeric
> >>>> arguments (and results) regardless of the domain of the value.
> >>>>
> >>>> Related, I've always worried about 'bool'....
> >>>>
> >>>>> The upper 32 bits of registers are always undefined garbage for types
> >>>>> smaller than 64 bits."
> >>>>
> >>>> On x86-64 the high bits are zeroed by all 32bit loads.
> >>>
> >>> Yeah, but does not help here.
> >>>
> >>>
> >>> My thinking: if the compiler that calls import_iovec() has garbage in
> >>> the upper 32 bit
> >>>
> >>> a) gcc will zero it out and not rely on it being zero.
> >>> b) clang will not zero it out, assuming it is zero.
> >>>
> >>> But
> >>>
> >>> a) will zero it out when calling the !inlined variant
> >>> b) clang will zero it out when calling the !inlined variant
> >>>
> >>> When inlining, b) strikes. We access garbage. That would mean that we
> >>> have calling code that's not generated by clang/gcc IIUC.
> >>>
> >>> We can test easily by changing the parameters instead of adding an "inline".
> >>
> >> Let me try that as well, as I seem to have a good reproducer, but it
> >> takes a while to run...
> >
> > Ok, that didn't work.
> >
> > And I can't seem to "fix" this by adding noinline to patches further
> > along in the patch series (because this commit's function is no longer
> > present due to later patches.)
>
> We might have the same issues with iovec_from_user() and friends now.
>
> >
> > Will keep digging...
> >
> > greg k-h
> >
>
>
> Might be worth to give this a try, just to see if it's related to
> garbage in upper 32 bit and the way clang is handling it (might be a BUG
> in clang, though):
>
>
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 72d88566694e..7527298c6b56 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -267,7 +267,7 @@ size_t hash_and_copy_to_iter(const void *addr,
> size_t bytes, void *hashp,
> struct iov_iter *i);
>
> struct iovec *iovec_from_user(const struct iovec __user *uvector,
> - unsigned long nr_segs, unsigned long fast_segs,
> + unsigned nr_segs, unsigned fast_segs,
> struct iovec *fast_iov, bool compat);
> ssize_t import_iovec(int type, const struct iovec __user *uvec,
> unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 1635111c5bd2..58417f1916dc 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1652,7 +1652,7 @@ const void *dup_iter(struct iov_iter *new, struct
> iov_iter *old, gfp_t flags)
> EXPORT_SYMBOL(dup_iter);
>
> static int copy_compat_iovec_from_user(struct iovec *iov,
> - const struct iovec __user *uvec, unsigned long nr_segs)
> + const struct iovec __user *uvec, unsigned nr_segs)
> {
> const struct compat_iovec __user *uiov =
> (const struct compat_iovec __user *)uvec;
> @@ -1684,7 +1684,7 @@ static int copy_compat_iovec_from_user(struct
> iovec *iov,
> }
>
> static int copy_iovec_from_user(struct iovec *iov,
> - const struct iovec __user *uvec, unsigned long nr_segs)
> + const struct iovec __user *uvec, unsigned nr_segs)
> {
> unsigned long seg;
>
> @@ -1699,7 +1699,7 @@ static int copy_iovec_from_user(struct iovec *iov,
> }
>
> struct iovec *iovec_from_user(const struct iovec __user *uvec,
> - unsigned long nr_segs, unsigned long fast_segs,
> + unsigned nr_segs, unsigned fast_segs,
> struct iovec *fast_iov, bool compat)
> {
> struct iovec *iov = fast_iov;
> @@ -1738,7 +1738,7 @@ ssize_t __import_iovec(int type, const struct
> iovec __user *uvec,
> struct iov_iter *i, bool compat)
> {
> ssize_t total_len = 0;
> - unsigned long seg;
> + unsigned seg;
> struct iovec *iov;
>
> iov = iovec_from_user(uvec, nr_segs, fast_segs, *iovp, compat);
>
Ah, I tested the other way around, making everything "unsigned long"
instead. Will go try this too, as other tests are still running...
thanks,
greg k-h
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Greg KH @ 2020-10-22 13:50 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022125759.GA1685526@kroah.com>
On Thu, Oct 22, 2020 at 02:57:59PM +0200, Greg KH wrote:
> On Thu, Oct 22, 2020 at 02:42:24PM +0200, David Hildenbrand wrote:
> > On 22.10.20 14:18, Greg KH wrote:
> > > On Thu, Oct 22, 2020 at 12:48:05PM +0200, Greg KH wrote:
> > >> On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
> > >>> On 22.10.20 11:32, David Laight wrote:
> > >>>> From: David Hildenbrand
> > >>>>> Sent: 22 October 2020 10:25
> > >>>> ...
> > >>>>> ... especially because I recall that clang and gcc behave slightly
> > >>>>> differently:
> > >>>>>
> > >>>>> https://github.com/hjl-tools/x86-psABI/issues/2
> > >>>>>
> > >>>>> "Function args are different: narrow types are sign or zero extended to
> > >>>>> 32 bits, depending on their type. clang depends on this for incoming
> > >>>>> args, but gcc doesn't make that assumption. But both compilers do it
> > >>>>> when calling, so gcc code can call clang code.
> > >>>>
> > >>>> It really is best to use 'int' (or even 'long') for all numeric
> > >>>> arguments (and results) regardless of the domain of the value.
> > >>>>
> > >>>> Related, I've always worried about 'bool'....
> > >>>>
> > >>>>> The upper 32 bits of registers are always undefined garbage for types
> > >>>>> smaller than 64 bits."
> > >>>>
> > >>>> On x86-64 the high bits are zeroed by all 32bit loads.
> > >>>
> > >>> Yeah, but does not help here.
> > >>>
> > >>>
> > >>> My thinking: if the compiler that calls import_iovec() has garbage in
> > >>> the upper 32 bit
> > >>>
> > >>> a) gcc will zero it out and not rely on it being zero.
> > >>> b) clang will not zero it out, assuming it is zero.
> > >>>
> > >>> But
> > >>>
> > >>> a) will zero it out when calling the !inlined variant
> > >>> b) clang will zero it out when calling the !inlined variant
> > >>>
> > >>> When inlining, b) strikes. We access garbage. That would mean that we
> > >>> have calling code that's not generated by clang/gcc IIUC.
> > >>>
> > >>> We can test easily by changing the parameters instead of adding an "inline".
> > >>
> > >> Let me try that as well, as I seem to have a good reproducer, but it
> > >> takes a while to run...
> > >
> > > Ok, that didn't work.
> > >
> > > And I can't seem to "fix" this by adding noinline to patches further
> > > along in the patch series (because this commit's function is no longer
> > > present due to later patches.)
> >
> > We might have the same issues with iovec_from_user() and friends now.
> >
> > >
> > > Will keep digging...
> > >
> > > greg k-h
> > >
> >
> >
> > Might be worth to give this a try, just to see if it's related to
> > garbage in upper 32 bit and the way clang is handling it (might be a BUG
> > in clang, though):
> >
> >
> > diff --git a/include/linux/uio.h b/include/linux/uio.h
> > index 72d88566694e..7527298c6b56 100644
> > --- a/include/linux/uio.h
> > +++ b/include/linux/uio.h
> > @@ -267,7 +267,7 @@ size_t hash_and_copy_to_iter(const void *addr,
> > size_t bytes, void *hashp,
> > struct iov_iter *i);
> >
> > struct iovec *iovec_from_user(const struct iovec __user *uvector,
> > - unsigned long nr_segs, unsigned long fast_segs,
> > + unsigned nr_segs, unsigned fast_segs,
> > struct iovec *fast_iov, bool compat);
> > ssize_t import_iovec(int type, const struct iovec __user *uvec,
> > unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
> > diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> > index 1635111c5bd2..58417f1916dc 100644
> > --- a/lib/iov_iter.c
> > +++ b/lib/iov_iter.c
> > @@ -1652,7 +1652,7 @@ const void *dup_iter(struct iov_iter *new, struct
> > iov_iter *old, gfp_t flags)
> > EXPORT_SYMBOL(dup_iter);
> >
> > static int copy_compat_iovec_from_user(struct iovec *iov,
> > - const struct iovec __user *uvec, unsigned long nr_segs)
> > + const struct iovec __user *uvec, unsigned nr_segs)
> > {
> > const struct compat_iovec __user *uiov =
> > (const struct compat_iovec __user *)uvec;
> > @@ -1684,7 +1684,7 @@ static int copy_compat_iovec_from_user(struct
> > iovec *iov,
> > }
> >
> > static int copy_iovec_from_user(struct iovec *iov,
> > - const struct iovec __user *uvec, unsigned long nr_segs)
> > + const struct iovec __user *uvec, unsigned nr_segs)
> > {
> > unsigned long seg;
> >
> > @@ -1699,7 +1699,7 @@ static int copy_iovec_from_user(struct iovec *iov,
> > }
> >
> > struct iovec *iovec_from_user(const struct iovec __user *uvec,
> > - unsigned long nr_segs, unsigned long fast_segs,
> > + unsigned nr_segs, unsigned fast_segs,
> > struct iovec *fast_iov, bool compat)
> > {
> > struct iovec *iov = fast_iov;
> > @@ -1738,7 +1738,7 @@ ssize_t __import_iovec(int type, const struct
> > iovec __user *uvec,
> > struct iov_iter *i, bool compat)
> > {
> > ssize_t total_len = 0;
> > - unsigned long seg;
> > + unsigned seg;
> > struct iovec *iov;
> >
> > iov = iovec_from_user(uvec, nr_segs, fast_segs, *iovp, compat);
> >
>
> Ah, I tested the other way around, making everything "unsigned long"
> instead. Will go try this too, as other tests are still running...
Ok, no, this didn't work either.
Nick, I think I need some compiler help here. Any ideas?
thanks,
greg k-h
^ permalink raw reply
* [PATCH] powerpc/bitops: Fix possible undefined behaviour with fls() and fls64()
From: Christophe Leroy @ 2020-10-22 14:05 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, jakub,
segher
Cc: linuxppc-dev, linux-kernel
fls() and fls64() are using __builtin_ctz() and _builtin_ctzll().
On powerpc, those builtins trivially use ctlzw and ctlzd power
instructions.
Allthough those instructions provide the expected result with
input argument 0, __builtin_ctz() and __builtin_ctzll() are
documented as undefined for value 0.
The easiest fix would be to use fls() and fls64() functions
defined in include/asm-generic/bitops/builtin-fls.h and
include/asm-generic/bitops/fls64.h, but GCC output is not optimal:
00000388 <testfls>:
388: 2c 03 00 00 cmpwi r3,0
38c: 41 82 00 10 beq 39c <testfls+0x14>
390: 7c 63 00 34 cntlzw r3,r3
394: 20 63 00 20 subfic r3,r3,32
398: 4e 80 00 20 blr
39c: 38 60 00 00 li r3,0
3a0: 4e 80 00 20 blr
000003b0 <testfls64>:
3b0: 2c 03 00 00 cmpwi r3,0
3b4: 40 82 00 1c bne 3d0 <testfls64+0x20>
3b8: 2f 84 00 00 cmpwi cr7,r4,0
3bc: 38 60 00 00 li r3,0
3c0: 4d 9e 00 20 beqlr cr7
3c4: 7c 83 00 34 cntlzw r3,r4
3c8: 20 63 00 20 subfic r3,r3,32
3cc: 4e 80 00 20 blr
3d0: 7c 63 00 34 cntlzw r3,r3
3d4: 20 63 00 40 subfic r3,r3,64
3d8: 4e 80 00 20 blr
When the input of fls(x) is a constant, just check x for nullity and
return either 0 or __builtin_clz(x). Otherwise, use cntlzw instruction
directly.
For fls64() on PPC64, do the same but with __builtin_clzll() and
cntlzd instruction. On PPC32, lets take the generic fls64() which
will use our fls(). The result is as expected:
00000388 <testfls>:
388: 7c 63 00 34 cntlzw r3,r3
38c: 20 63 00 20 subfic r3,r3,32
390: 4e 80 00 20 blr
000003a0 <testfls64>:
3a0: 2c 03 00 00 cmpwi r3,0
3a4: 40 82 00 10 bne 3b4 <testfls64+0x14>
3a8: 7c 83 00 34 cntlzw r3,r4
3ac: 20 63 00 20 subfic r3,r3,32
3b0: 4e 80 00 20 blr
3b4: 7c 63 00 34 cntlzw r3,r3
3b8: 20 63 00 40 subfic r3,r3,64
3bc: 4e 80 00 20 blr
Fixes: 2fcff790dcb4 ("powerpc: Use builtin functions for fls()/__fls()/fls64()")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/bitops.h | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/bitops.h b/arch/powerpc/include/asm/bitops.h
index 4a4d3afd5340..299ab33505a6 100644
--- a/arch/powerpc/include/asm/bitops.h
+++ b/arch/powerpc/include/asm/bitops.h
@@ -216,15 +216,34 @@ static inline void arch___clear_bit_unlock(int nr, volatile unsigned long *addr)
*/
static inline int fls(unsigned int x)
{
- return 32 - __builtin_clz(x);
+ int lz;
+
+ if (__builtin_constant_p(x))
+ return x ? 32 - __builtin_clz(x) : 0;
+ asm("cntlzw %0,%1" : "=r" (lz) : "r" (x));
+ return 32 - lz;
}
#include <asm-generic/bitops/builtin-__fls.h>
+/*
+ * 64-bit can do this using one cntlzd (count leading zeroes doubleword)
+ * instruction; for 32-bit we use the generic version, which does two
+ * 32-bit fls calls.
+ */
+#ifdef CONFIG_PPC64
static inline int fls64(__u64 x)
{
- return 64 - __builtin_clzll(x);
+ int lz;
+
+ if (__builtin_constant_p(x))
+ return x ? 64 - __builtin_clzll(x) : 0;
+ asm("cntlzd %0,%1" : "=r" (lz) : "r" (x));
+ return 64 - lz;
}
+#else
+#include <asm-generic/bitops/fls64.h>
+#endif
#ifdef CONFIG_PPC64
unsigned int __arch_hweight8(unsigned int w);
--
2.25.0
^ permalink raw reply related
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Arnd Bergmann @ 2020-10-22 14:28 UTC (permalink / raw)
To: Greg KH
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022135036.GA1787470@kroah.com>
On Thu, Oct 22, 2020 at 3:50 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Oct 22, 2020 at 02:57:59PM +0200, Greg KH wrote:
> > On Thu, Oct 22, 2020 at 02:42:24PM +0200, David Hildenbrand wrote:
> > > struct iovec *iovec_from_user(const struct iovec __user *uvec,
> > > - unsigned long nr_segs, unsigned long fast_segs,
> > > + unsigned nr_segs, unsigned fast_segs,
> > > struct iovec *fast_iov, bool compat)
> > > {
> > > struct iovec *iov = fast_iov;
> > > @@ -1738,7 +1738,7 @@ ssize_t __import_iovec(int type, const struct
> > > iovec __user *uvec,
> > > struct iov_iter *i, bool compat)
> > > {
> > > ssize_t total_len = 0;
> > > - unsigned long seg;
> > > + unsigned seg;
> > > struct iovec *iov;
> > >
> > > iov = iovec_from_user(uvec, nr_segs, fast_segs, *iovp, compat);
> > >
> >
> > Ah, I tested the other way around, making everything "unsigned long"
> > instead. Will go try this too, as other tests are still running...
>
> Ok, no, this didn't work either.
>
> Nick, I think I need some compiler help here. Any ideas?
I don't think the patch above would reliably clear the upper bits if they
contain garbage.
If the integer extension is the problem, the way I'd try it is to make the
function take an 'unsigned long' and then explictly mask the upper
bits with
seg = lower_32_bits(seg);
Can you attach the iov_iter.s files from the broken build, plus the
one with 'noinline' for comparison? Maybe something can be seen
in there.
Arnd
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Greg KH @ 2020-10-22 14:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <CAK8P3a1B7OVdyzW0-97JwzZiwp0D0fnSfyete16QTvPp_1m07A@mail.gmail.com>
On Thu, Oct 22, 2020 at 04:28:20PM +0200, Arnd Bergmann wrote:
> On Thu, Oct 22, 2020 at 3:50 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Oct 22, 2020 at 02:57:59PM +0200, Greg KH wrote:
> > > On Thu, Oct 22, 2020 at 02:42:24PM +0200, David Hildenbrand wrote:
>
> > > > struct iovec *iovec_from_user(const struct iovec __user *uvec,
> > > > - unsigned long nr_segs, unsigned long fast_segs,
> > > > + unsigned nr_segs, unsigned fast_segs,
> > > > struct iovec *fast_iov, bool compat)
> > > > {
> > > > struct iovec *iov = fast_iov;
> > > > @@ -1738,7 +1738,7 @@ ssize_t __import_iovec(int type, const struct
> > > > iovec __user *uvec,
> > > > struct iov_iter *i, bool compat)
> > > > {
> > > > ssize_t total_len = 0;
> > > > - unsigned long seg;
> > > > + unsigned seg;
> > > > struct iovec *iov;
> > > >
> > > > iov = iovec_from_user(uvec, nr_segs, fast_segs, *iovp, compat);
> > > >
> > >
> > > Ah, I tested the other way around, making everything "unsigned long"
> > > instead. Will go try this too, as other tests are still running...
> >
> > Ok, no, this didn't work either.
> >
> > Nick, I think I need some compiler help here. Any ideas?
>
> I don't think the patch above would reliably clear the upper bits if they
> contain garbage.
>
> If the integer extension is the problem, the way I'd try it is to make the
> function take an 'unsigned long' and then explictly mask the upper
> bits with
>
> seg = lower_32_bits(seg);
>
> Can you attach the iov_iter.s files from the broken build, plus the
> one with 'noinline' for comparison? Maybe something can be seen
> in there.
I don't know how to extract the .s files easily from the AOSP build
system, I'll look into that. I'm also now testing by downgrading to an
older version of clang (10 instead of 11), to see if that matters at all
or not...
thanks,
greg k-h
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Christoph Hellwig @ 2020-10-22 13:23 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, Greg KH,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <5fd6003b-55a6-2c3c-9a28-8fd3a575ca78@redhat.com>
On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
> My thinking: if the compiler that calls import_iovec() has garbage in
> the upper 32 bit
>
> a) gcc will zero it out and not rely on it being zero.
> b) clang will not zero it out, assuming it is zero.
>
> But
>
> a) will zero it out when calling the !inlined variant
> b) clang will zero it out when calling the !inlined variant
>
> When inlining, b) strikes. We access garbage. That would mean that we
> have calling code that's not generated by clang/gcc IIUC.
Most callchains of import_iovec start with the assembly syscall wrappers.
^ permalink raw reply
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-22 16:15 UTC (permalink / raw)
To: 'Greg KH', Arnd Bergmann
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022144021.GA1969554@kroah.com>
From: Greg KH
> Sent: 22 October 2020 15:40
>
> On Thu, Oct 22, 2020 at 04:28:20PM +0200, Arnd Bergmann wrote:
...
> > Can you attach the iov_iter.s files from the broken build, plus the
> > one with 'noinline' for comparison? Maybe something can be seen
> > in there.
>
> I don't know how to extract the .s files easily from the AOSP build
> system, I'll look into that. I'm also now testing by downgrading to an
> older version of clang (10 instead of 11), to see if that matters at all
> or not...
Back from a day out - after it stopped raining.
Trying to use up leave before the end of the year.
Can you use objdump on the kernel binary itself and cut out
the single function?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-22 16:35 UTC (permalink / raw)
To: 'Christoph Hellwig', David Hildenbrand
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, Al Viro, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, Greg KH, Nick Desaulniers,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022132342.GB8781@lst.de>
From: Christoph Hellwig
> Sent: 22 October 2020 14:24
>
> On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
> > My thinking: if the compiler that calls import_iovec() has garbage in
> > the upper 32 bit
> >
> > a) gcc will zero it out and not rely on it being zero.
> > b) clang will not zero it out, assuming it is zero.
> >
> > But
> >
> > a) will zero it out when calling the !inlined variant
> > b) clang will zero it out when calling the !inlined variant
> >
> > When inlining, b) strikes. We access garbage. That would mean that we
> > have calling code that's not generated by clang/gcc IIUC.
>
> Most callchains of import_iovec start with the assembly syscall wrappers.
Wait...
readv(2) defines:
ssize_t readv(int fd, const struct iovec *iov, int iovcnt);
But the syscall is defined as:
SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
unsigned long, vlen)
{
return do_readv(fd, vec, vlen, 0);
}
I'm guessing that nothing actually masks the high bits that come
from an application that is compiled with clang?
The vlen is 'unsigned long' through the first few calls.
So unless there is a non-inlined function than takes vlen
as 'int' the high garbage bits from userspace are kept.
Which makes it a bug in the kernel C syscall wrappers.
They need to explicitly mask the high bits of 32bit
arguments on arm64 but not x86-64.
What does the ARM EABI say about register parameters?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH] powerpc/bitops: Fix possible undefined behaviour with fls() and fls64()
From: Segher Boessenkool @ 2020-10-22 16:37 UTC (permalink / raw)
To: Christophe Leroy; +Cc: jakub, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <348c2d3f19ffcff8abe50d52513f989c4581d000.1603375524.git.christophe.leroy@csgroup.eu>
On Thu, Oct 22, 2020 at 02:05:46PM +0000, Christophe Leroy wrote:
> fls() and fls64() are using __builtin_ctz() and _builtin_ctzll().
> On powerpc, those builtins trivially use ctlzw and ctlzd power
> instructions.
>
> Allthough those instructions provide the expected result with
> input argument 0, __builtin_ctz() and __builtin_ctzll() are
> documented as undefined for value 0.
> When the input of fls(x) is a constant, just check x for nullity and
> return either 0 or __builtin_clz(x). Otherwise, use cntlzw instruction
> directly.
That looks good :-)
Acked-by: Segher Boessenkool <segher@kernel.crashing.org>
Segher
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Matthew Wilcox @ 2020-10-22 16:40 UTC (permalink / raw)
To: David Laight
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
'Christoph Hellwig', linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, Al Viro, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, Greg KH, Nick Desaulniers,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <8f1fff0c358b4b669d51cc80098dbba1@AcuMS.aculab.com>
On Thu, Oct 22, 2020 at 04:35:17PM +0000, David Laight wrote:
> Wait...
> readv(2) defines:
> ssize_t readv(int fd, const struct iovec *iov, int iovcnt);
It doesn't really matter what the manpage says. What does the AOSP
libc header say?
> But the syscall is defined as:
>
> SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
> unsigned long, vlen)
> {
> return do_readv(fd, vec, vlen, 0);
> }
^ permalink raw reply
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-22 16:50 UTC (permalink / raw)
To: 'Matthew Wilcox'
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
'Christoph Hellwig', linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, Al Viro, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, Greg KH, Nick Desaulniers,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022164040.GV20115@casper.infradead.org>
From: Matthew Wilcox <willy@infradead.org>
> Sent: 22 October 2020 17:41
>
> On Thu, Oct 22, 2020 at 04:35:17PM +0000, David Laight wrote:
> > Wait...
> > readv(2) defines:
> > ssize_t readv(int fd, const struct iovec *iov, int iovcnt);
>
> It doesn't really matter what the manpage says. What does the AOSP
> libc header say?
The only copy I can find is:
/usr/include/x86_64-linux-gnu/sys/uio.h:extern ssize_t readv (int __fd, const struct iovec *__iovec, int __count)
/usr/include/x86_64-linux-gnu/sys/uio.h- __wur;
and not surprisingly agrees.
POSIX and/or TOG will (more or less) mandate the prototype.
> > But the syscall is defined as:
> >
> > SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
> > unsigned long, vlen)
> > {
> > return do_readv(fd, vec, vlen, 0);
> > }
I wonder when the high bits of 'fd' get zapped?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Nick Desaulniers @ 2020-10-22 17:00 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, Al Viro, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, Greg KH,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022164040.GV20115@casper.infradead.org>
On Thu, Oct 22, 2020 at 9:40 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Oct 22, 2020 at 04:35:17PM +0000, David Laight wrote:
> > Wait...
> > readv(2) defines:
> > ssize_t readv(int fd, const struct iovec *iov, int iovcnt);
>
> It doesn't really matter what the manpage says. What does the AOSP
> libc header say?
Same: https://android.googlesource.com/platform/bionic/+/refs/heads/master/libc/include/sys/uio.h#38
Theoretically someone could bypass libc to make a system call, right?
>
> > But the syscall is defined as:
> >
> > SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
> > unsigned long, vlen)
> > {
> > return do_readv(fd, vec, vlen, 0);
> > }
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Nick Desaulniers @ 2020-10-22 17:54 UTC (permalink / raw)
To: David Laight
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, Al Viro, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, Greg KH,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <8f1fff0c358b4b669d51cc80098dbba1@AcuMS.aculab.com>
[-- Attachment #1: Type: text/plain, Size: 2086 bytes --]
On Thu, Oct 22, 2020 at 9:35 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Christoph Hellwig
> > Sent: 22 October 2020 14:24
> >
> > On Thu, Oct 22, 2020 at 11:36:40AM +0200, David Hildenbrand wrote:
> > > My thinking: if the compiler that calls import_iovec() has garbage in
> > > the upper 32 bit
> > >
> > > a) gcc will zero it out and not rely on it being zero.
> > > b) clang will not zero it out, assuming it is zero.
> > >
> > > But
> > >
> > > a) will zero it out when calling the !inlined variant
> > > b) clang will zero it out when calling the !inlined variant
> > >
> > > When inlining, b) strikes. We access garbage. That would mean that we
> > > have calling code that's not generated by clang/gcc IIUC.
> >
> > Most callchains of import_iovec start with the assembly syscall wrappers.
>
> Wait...
> readv(2) defines:
> ssize_t readv(int fd, const struct iovec *iov, int iovcnt);
>
> But the syscall is defined as:
>
> SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
> unsigned long, vlen)
> {
> return do_readv(fd, vec, vlen, 0);
> }
>
> I'm guessing that nothing actually masks the high bits that come
> from an application that is compiled with clang?
>
> The vlen is 'unsigned long' through the first few calls.
> So unless there is a non-inlined function than takes vlen
> as 'int' the high garbage bits from userspace are kept.
Yeah, that's likely a bug: https://godbolt.org/z/KfsPKs
>
> Which makes it a bug in the kernel C syscall wrappers.
> They need to explicitly mask the high bits of 32bit
> arguments on arm64 but not x86-64.
Why not x86-64? Wouldn't it be *any* LP64 ISA?
Attaching a patch that uses the proper width, but I'm pretty sure
there's still a signedness issue . Greg, would you mind running this
through the wringer?
>
> What does the ARM EABI say about register parameters?
AAPCS is the ABI for 64b ARM, IIUC, which is the ISA GKH is reporting
the problem against. IIUC, EABI is one of the 32b ABIs. aarch64 is
LP64 just like x86_64.
--
Thanks,
~Nick Desaulniers
[-- Attachment #2: 0001-fs-fix-up-type-confusion-in-readv-writev.patch --]
[-- Type: application/octet-stream, Size: 4630 bytes --]
From aae26b13ffb9e38bb46b8c85985761b5f196b6f6 Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <ndesaulniers@google.com>
Date: Thu, 22 Oct 2020 10:23:47 -0700
Subject: [PATCH] fs: fix up type confusion in readv/writev
The syscall interface doesn't match up with the interface libc is using
or that's defined in the manual pages.
ssize_t readv(int fd, const struct iovec *iov, int iovcnt);
ssize_t writev(int fd, const struct iovec *iov, int iovcnt);
The kernel was defining `iovcnt` as `unsigned long` which is a problem
when userspace understands this to be `int`.
(There's still likely a signedness bug here, but use the proper widths
that import_iovec() expects.)
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
fs/read_write.c | 10 +++++-----
fs/splice.c | 2 +-
include/linux/fs.h | 2 +-
lib/iov_iter.c | 4 ++--
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index 19f5c4bf75aa..b858f39a4475 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -890,7 +890,7 @@ ssize_t vfs_iter_write(struct file *file, struct iov_iter *iter, loff_t *ppos,
EXPORT_SYMBOL(vfs_iter_write);
ssize_t vfs_readv(struct file *file, const struct iovec __user *vec,
- unsigned long vlen, loff_t *pos, rwf_t flags)
+ unsigned int vlen, loff_t *pos, rwf_t flags)
{
struct iovec iovstack[UIO_FASTIOV];
struct iovec *iov = iovstack;
@@ -907,7 +907,7 @@ ssize_t vfs_readv(struct file *file, const struct iovec __user *vec,
}
static ssize_t vfs_writev(struct file *file, const struct iovec __user *vec,
- unsigned long vlen, loff_t *pos, rwf_t flags)
+ unsigned int vlen, loff_t *pos, rwf_t flags)
{
struct iovec iovstack[UIO_FASTIOV];
struct iovec *iov = iovstack;
@@ -925,7 +925,7 @@ static ssize_t vfs_writev(struct file *file, const struct iovec __user *vec,
}
static ssize_t do_readv(unsigned long fd, const struct iovec __user *vec,
- unsigned long vlen, rwf_t flags)
+ unsigned int vlen, rwf_t flags)
{
struct fd f = fdget_pos(fd);
ssize_t ret = -EBADF;
@@ -1025,13 +1025,13 @@ static ssize_t do_pwritev(unsigned long fd, const struct iovec __user *vec,
}
SYSCALL_DEFINE3(readv, unsigned long, fd, const struct iovec __user *, vec,
- unsigned long, vlen)
+ unsigned int, vlen)
{
return do_readv(fd, vec, vlen, 0);
}
SYSCALL_DEFINE3(writev, unsigned long, fd, const struct iovec __user *, vec,
- unsigned long, vlen)
+ unsigned int, vlen)
{
return do_writev(fd, vec, vlen, 0);
}
diff --git a/fs/splice.c b/fs/splice.c
index 70cc52af780b..7508eccfa143 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -342,7 +342,7 @@ const struct pipe_buf_operations nosteal_pipe_buf_ops = {
EXPORT_SYMBOL(nosteal_pipe_buf_ops);
static ssize_t kernel_readv(struct file *file, const struct kvec *vec,
- unsigned long vlen, loff_t offset)
+ unsigned int vlen, loff_t offset)
{
mm_segment_t old_fs;
loff_t pos = offset;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c4ae9cafbbba..211bce5e6e60 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1895,7 +1895,7 @@ static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);
extern ssize_t vfs_write(struct file *, const char __user *, size_t, loff_t *);
extern ssize_t vfs_readv(struct file *, const struct iovec __user *,
- unsigned long, loff_t *, rwf_t);
+ unsigned int, loff_t *, rwf_t);
extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
loff_t, size_t, unsigned int);
extern ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 1635111c5bd2..ded9d9c4eb28 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1734,7 +1734,7 @@ struct iovec *iovec_from_user(const struct iovec __user *uvec,
}
ssize_t __import_iovec(int type, const struct iovec __user *uvec,
- unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
+ unsigned int nr_segs, unsigned int fast_segs, struct iovec **iovp,
struct iov_iter *i, bool compat)
{
ssize_t total_len = 0;
@@ -1803,7 +1803,7 @@ ssize_t __import_iovec(int type, const struct iovec __user *uvec,
* Return: Negative error code on error, bytes imported on success
*/
ssize_t import_iovec(int type, const struct iovec __user *uvec,
- unsigned nr_segs, unsigned fast_segs,
+ unsigned int nr_segs, unsigned int fast_segs,
struct iovec **iovp, struct iov_iter *i)
{
return __import_iovec(type, uvec, nr_segs, fast_segs, iovp, i,
--
2.29.0.rc1.297.gfa9743e501-goog
^ permalink raw reply related
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Arnd Bergmann @ 2020-10-22 18:12 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, Greg KH,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <CAKwvOdnix6YGFhsmT_mY8ORNPTOsN3HwS33Dr0Ykn-pyJ6e-Bw@mail.gmail.com>
On Thu, Oct 22, 2020 at 7:54 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
> On Thu, Oct 22, 2020 at 9:35 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > Which makes it a bug in the kernel C syscall wrappers.
> > They need to explicitly mask the high bits of 32bit
> > arguments on arm64 but not x86-64.
>
> Why not x86-64? Wouldn't it be *any* LP64 ISA?
x86-64 is slightly special because most instructions on a 32-bit
argument clear the upper 32 bits, while on most architectures
the same instruction would leave the upper bits unchanged.
> Attaching a patch that uses the proper width, but I'm pretty sure
> there's still a signedness issue . Greg, would you mind running this
> through the wringer?
I would not expect this to change anything for the bug that Greg
is chasing, unless there is also a bug in clang.
In the version before the patch, we get a 64-bit argument from
user space, which may consist of the intended value in the lower
bits plus garbage in the upper bits. However, vlen only gets
passed down into import_iovec() without any other operations
on it, and ince import_iovec takes a 32-bit argument, this is
where it finally gets narrowed.
After your patch, the SYSCALL_DEFINE3() does the narrowing
conversion with the same clearing of the upper bits.
If there is a problem somewhere leading up to import_iovec(),
it would have to in some code that expects to get a 32-bit
register argument but gets called with a register that has
garbage in the upper bits /without/ going through a correct
sanitizing function like SYSCALL_DEFINE3().
Arnd
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Al Viro @ 2020-10-22 18:19 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
'Christoph Hellwig', linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, Greg KH, Nick Desaulniers,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022164040.GV20115@casper.infradead.org>
On Thu, Oct 22, 2020 at 05:40:40PM +0100, Matthew Wilcox wrote:
> On Thu, Oct 22, 2020 at 04:35:17PM +0000, David Laight wrote:
> > Wait...
> > readv(2) defines:
> > ssize_t readv(int fd, const struct iovec *iov, int iovcnt);
>
> It doesn't really matter what the manpage says. What does the AOSP
> libc header say?
FWIW, see https://www.spinics.net/lists/linux-scsi/msg147836.html and
subthread from there on...
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Nick Desaulniers @ 2020-10-22 19:04 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, Greg KH,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <CAK8P3a3LjG+ZvmQrkb9zpgov8xBkQQWrkHBPgjfYSqBKGrwT4w@mail.gmail.com>
On Thu, Oct 22, 2020 at 11:13 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Oct 22, 2020 at 7:54 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> > On Thu, Oct 22, 2020 at 9:35 AM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > Which makes it a bug in the kernel C syscall wrappers.
> > > They need to explicitly mask the high bits of 32bit
> > > arguments on arm64 but not x86-64.
> >
> > Why not x86-64? Wouldn't it be *any* LP64 ISA?
>
> x86-64 is slightly special because most instructions on a 32-bit
> argument clear the upper 32 bits, while on most architectures
> the same instruction would leave the upper bits unchanged.
Oh interesting, depends on the operations too on x86_64 IIUC?
>
> > Attaching a patch that uses the proper width, but I'm pretty sure
> > there's still a signedness issue . Greg, would you mind running this
> > through the wringer?
>
> I would not expect this to change anything for the bug that Greg
> is chasing, unless there is also a bug in clang.
>
> In the version before the patch, we get a 64-bit argument from
> user space, which may consist of the intended value in the lower
> bits plus garbage in the upper bits. However, vlen only gets
> passed down into import_iovec() without any other operations
> on it, and since import_iovec takes a 32-bit argument, this is
> where it finally gets narrowed.
Passing an `unsigned long` as an `unsigned int` does no such
narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
calls, no masking instructions).
So if rw_copy_check_uvector() is inlined into import_iovec() (looking
at the mainline@1028ae406999), then children calls of
`rw_copy_check_uvector()` will be interpreting the `nr_segs` register
unmodified, ie. garbage in the upper 32b.
>
> After your patch, the SYSCALL_DEFINE3() does the narrowing
> conversion with the same clearing of the upper bits.
>
> If there is a problem somewhere leading up to import_iovec(),
> it would have to in some code that expects to get a 32-bit
> register argument but gets called with a register that has
> garbage in the upper bits /without/ going through a correct
> sanitizing function like SYSCALL_DEFINE3().
>
> Arnd
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Al Viro @ 2020-10-22 19:24 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, Greg KH,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <CAKwvOdnhONvrHLAuz_BrAuEpnF5mD9p0YPGJs=NZZ0EZNo7dFQ@mail.gmail.com>
On Thu, Oct 22, 2020 at 12:04:52PM -0700, Nick Desaulniers wrote:
> Passing an `unsigned long` as an `unsigned int` does no such
> narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
> calls, no masking instructions).
> So if rw_copy_check_uvector() is inlined into import_iovec() (looking
> at the mainline@1028ae406999), then children calls of
> `rw_copy_check_uvector()` will be interpreting the `nr_segs` register
> unmodified, ie. garbage in the upper 32b.
FWIW,
void f(unsinged long v)
{
if (v != 1)
printf("failed\n");
}
void g(unsigned int v)
{
f(v);
}
void h(unsigned long v)
{
g(v);
}
main()
{
h(0x100000001);
}
must not produce any output on a host with 32bit int and 64bit long, regardless of
the inlining, having functions live in different compilation units, etc.
Depending upon the calling conventions, compiler might do truncation in caller or
in a callee, but it must be done _somewhere_.
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Al Viro @ 2020-10-22 19:27 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, Greg KH,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022192458.GV3576660@ZenIV.linux.org.uk>
On Thu, Oct 22, 2020 at 08:24:58PM +0100, Al Viro wrote:
> On Thu, Oct 22, 2020 at 12:04:52PM -0700, Nick Desaulniers wrote:
>
> > Passing an `unsigned long` as an `unsigned int` does no such
> > narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
> > calls, no masking instructions).
> > So if rw_copy_check_uvector() is inlined into import_iovec() (looking
> > at the mainline@1028ae406999), then children calls of
> > `rw_copy_check_uvector()` will be interpreting the `nr_segs` register
> > unmodified, ie. garbage in the upper 32b.
>
> FWIW,
>
> void f(unsinged long v)
s/unsinged/unsigned/, obviously...
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Al Viro @ 2020-10-22 20:06 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, Greg KH,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022192458.GV3576660@ZenIV.linux.org.uk>
On Thu, Oct 22, 2020 at 08:24:58PM +0100, Al Viro wrote:
> Depending upon the calling conventions, compiler might do truncation in caller or
> in a callee, but it must be done _somewhere_.
Unless I'm misreading AAPCS64,
"Unlike in the 32-bit AAPCS, named integral values must be narrowed by the callee
rather than the caller"
in 6.4.2 means that callee must not _not_ expect the upper 32 bits of %x0..%x7 to contain
anything valid for 32bit arguments and it must zero-extend %w0..%w7 when passing that to
something that expects a 64bit argument. On inlining it should be the same situation as
storing unsigned int argument into unsigned long local variable and working with that - if
void f(unsigned int w)
{
unsigned long x = w;
printf("%lx\n", x);
}
ends up passing %x0 to printf, it's an obvious bug - it must do something like
uxtw x0, w0
first.
What am I missing here?
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Arnd Bergmann @ 2020-10-22 20:06 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, Greg KH,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <CAKwvOdnhONvrHLAuz_BrAuEpnF5mD9p0YPGJs=NZZ0EZNo7dFQ@mail.gmail.com>
On Thu, Oct 22, 2020 at 9:05 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Oct 22, 2020 at 11:13 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Thu, Oct 22, 2020 at 7:54 PM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > > On Thu, Oct 22, 2020 at 9:35 AM David Laight <David.Laight@aculab.com> wrote:
> > > >
> > > > Which makes it a bug in the kernel C syscall wrappers.
> > > > They need to explicitly mask the high bits of 32bit
> > > > arguments on arm64 but not x86-64.
> > >
> > > Why not x86-64? Wouldn't it be *any* LP64 ISA?
> >
> > x86-64 is slightly special because most instructions on a 32-bit
> > argument clear the upper 32 bits, while on most architectures
> > the same instruction would leave the upper bits unchanged.
>
> Oh interesting, depends on the operations too on x86_64 IIUC?
It seems this doesn't impact the calling conventions (see below),
it's just that there are more cases on x86 where the callee doesn't
have to explicitly clear the upper bits because the this is implied.
> > > Attaching a patch that uses the proper width, but I'm pretty sure
> > > there's still a signedness issue . Greg, would you mind running this
> > > through the wringer?
> >
> > I would not expect this to change anything for the bug that Greg
> > is chasing, unless there is also a bug in clang.
> >
> > In the version before the patch, we get a 64-bit argument from
> > user space, which may consist of the intended value in the lower
> > bits plus garbage in the upper bits. However, vlen only gets
> > passed down into import_iovec() without any other operations
> > on it, and since import_iovec takes a 32-bit argument, this is
> > where it finally gets narrowed.
>
> Passing an `unsigned long` as an `unsigned int` does no such
> narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
> calls, no masking instructions).
Sorry I got it wrong, looked up the aarch64 AAPCS now, which
explains
"Unlike in the 32-bit AAPCS, named integral values must be
narrowed by the callee rather than the caller."
Also confirmed using https://godbolt.org/z/acPrjj, which
shows more combinations of compilers and architectures
in addition to your example. I had expected arm64 to be
like powerpc64 and arm32 in this case, but it's the reverse.
I also verified that SYSCALL_DEFINEx() is correct on arm64
and saw that as of v4.19 it passes the syscall arguments
through pt_regs, which will do the right thing here regardless
of the argument passing rules. The earlier version also seems
to be working as intended.
Arnd
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Al Viro @ 2020-10-22 20:09 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, Greg KH,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022200629.GX3576660@ZenIV.linux.org.uk>
On Thu, Oct 22, 2020 at 09:06:29PM +0100, Al Viro wrote:
> On Thu, Oct 22, 2020 at 08:24:58PM +0100, Al Viro wrote:
>
> > Depending upon the calling conventions, compiler might do truncation in caller or
> > in a callee, but it must be done _somewhere_.
>
> Unless I'm misreading AAPCS64,
> "Unlike in the 32-bit AAPCS, named integral values must be narrowed by the callee
> rather than the caller"
> in 6.4.2 means that callee must not _not_ expect the upper 32 bits of %x0..%x7 to contain
Sorry, artefact of editing - that's
"in 6.4.2 means that callee must _not_ expect the upper 32 bits of %x0..%x7 to contain"
obviously.
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Nick Desaulniers @ 2020-10-22 20:11 UTC (permalink / raw)
To: Al Viro
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, Greg KH,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022192458.GV3576660@ZenIV.linux.org.uk>
On Thu, Oct 22, 2020 at 12:25 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Oct 22, 2020 at 12:04:52PM -0700, Nick Desaulniers wrote:
>
> > Passing an `unsigned long` as an `unsigned int` does no such
> > narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
> > calls, no masking instructions).
> > So if rw_copy_check_uvector() is inlined into import_iovec() (looking
> > at the mainline@1028ae406999), then children calls of
> > `rw_copy_check_uvector()` will be interpreting the `nr_segs` register
> > unmodified, ie. garbage in the upper 32b.
>
> FWIW,
>
> void f(unsinged long v)
> {
> if (v != 1)
> printf("failed\n");
> }
>
> void g(unsigned int v)
> {
> f(v);
> }
>
> void h(unsigned long v)
> {
> g(v);
> }
>
> main()
> {
> h(0x100000001);
> }
A good/analogous example, but things get weird when the leaf node in
the call chain is inline asm: https://godbolt.org/z/s19TY5
(I'm not sure that's precisely what's going on here; I'll need to dive
more into the calls rw_copy_check_uvector() makes to see if there's
inline asm somewhere, pretty sure calls to get_user with `nr_regs`
exist).
>
> must not produce any output on a host with 32bit int and 64bit long, regardless of
> the inlining, having functions live in different compilation units, etc.
>
> Depending upon the calling conventions, compiler might do truncation in caller or
> in a callee, but it must be done _somewhere_.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH] treewide: Convert macro and uses of __section(foo) to __section("foo")
From: Nick Desaulniers @ 2020-10-22 20:42 UTC (permalink / raw)
To: Joe Perches
Cc: clang-built-linux, linuxppc-dev, Linus Torvalds, LKML,
Miguel Ojeda
In-Reply-To: <fe8abcc88cff676ead8ee48db1e993e63b0611c7.1603327264.git.joe@perches.com>
.On Wed, Oct 21, 2020 at 7:36 PM Joe Perches <joe@perches.com> wrote:
>
> Use a more generic form for __section that requires quotes to avoid
> complications with clang and gcc differences.
>
> Remove the quote operator # from compiler_attributes.h __section macro.
>
> Convert all unquoted __section(foo) uses to quoted __section("foo").
> Also convert __attribute__((section("foo"))) uses to __section("foo")
> even if the __attribute__ has multiple list entry forms.
>
> Conversion done using a script:
>
> Link: https://lore.kernel.org/lkml/75393e5ddc272dc7403de74d645e6c6e0f4e70eb.camel@perches.com/2-convert_section.pl
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>
> This conversion was previously submitted to -next last month
> https://lore.kernel.org/lkml/46f69161e60b802488ba8c8f3f8bbf922aa3b49b.camel@perches.com/
>
> Nick Desaulniers found a defect in the conversion of 2 boot files
> for powerpc, but no other defect was found for any other arch.
Untested, but:
Reviewed-by: Nick Desaulniers <ndesaulniers@gooogle.com>
Good job handling the trickier cases when the attribute was mixed with
others, and printing it in scripts/mod/modpost.c.
The only cases that *might* be similar to PPC are:
> arch/s390/boot/startup.c | 2 +-
> arch/x86/boot/compressed/pgtable_64.c | 2 +-
> arch/x86/purgatory/purgatory.c | 4 ++--
So a quick test of x86_64 and s390 would be good.
Thanks for the patch.
>
> The script was corrected to avoid converting these 2 files.
>
> There is no difference between the script output when run on today's -next
> and Linus' tree through commit f804b3159482, so this should be reasonable to
> apply now.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox