Re: [v2 PATCH 0/3] arch: mm, vdso: consolidate PAGE_SIZE definition
On Wed, Mar 06 2024 at 15:14, Arnd Bergmann wrote: > From: Arnd Bergmann > > Naresh noticed that the newly added usage of the PAGE_SIZE macro in > include/vdso/datapage.h introduced a build regression. I had an older > patch that I revived to have this defined through Kconfig rather than > through including asm/page.h, which is not allowed in vdso code. > > The vdso patch series now has a temporary workaround, but I still want to > get this into v6.9 so we can place the hack with CONFIG_PAGE_SIZE > in the vdso. Thank you for cleaning this up! tglx
Re: [PATCH v2 3/3] arch: define CONFIG_PAGE_SIZE_*KB on all architectures
On Wed, Mar 06 2024 at 15:14, Arnd Bergmann wrote: > From: Arnd Bergmann > > Most architectures only support a single hardcoded page size. In order > to ensure that each one of these sets the corresponding Kconfig symbols, > change over the PAGE_SHIFT definition to the common one and allow > only the hardware page size to be selected. > > Acked-by: Guo Ren > Acked-by: Heiko Carstens > Acked-by: Stafford Horne > Acked-by: Johannes Berg > Signed-off-by: Arnd Bergmann Reviewed-by: Thomas Gleixner
Re: [PATCH v2 2/3] arch: simplify architecture specific page size configuration
On Wed, Mar 06 2024 at 15:14, Arnd Bergmann wrote: > From: Arnd Bergmann > > arc, arm64, parisc and powerpc all have their own Kconfig symbols > in place of the common CONFIG_PAGE_SIZE_4KB symbols. Change these > so the common symbols are the ones that are actually used, while > leaving the arhcitecture specific ones as the user visible > place for configuring it, to avoid breaking user configs. > > Reviewed-by: Christophe Leroy (powerpc32) > Acked-by: Catalin Marinas > Acked-by: Helge Deller # parisc > Signed-off-by: Arnd Bergmann Reviewed-by: Thomas Gleixner
Re: [PATCH v2 1/3] arch: consolidate existing CONFIG_PAGE_SIZE_*KB definitions
On Wed, Mar 06 2024 at 15:14, Arnd Bergmann wrote: > From: Arnd Bergmann > > These four architectures define the same Kconfig symbols for configuring > the page size. Move the logic into a common place where it can be shared > with all other architectures. > > Signed-off-by: Arnd Bergmann Reviewed-by: Thomas Gleixner
Re: [PATCH] Drop unused isa_page_to_bus
On Thu, 13 Jun 2019, Stephen Kitt wrote: > isa_page_to_bus is deprecated and no longer used anywhere, this patch > removes it entirely. > > Signed-off-by: Stephen Kitt Acked-by: Thomas Gleixner
Re: [PATCH v2] time: Make sure jiffies_to_msecs() preserves non-zero time periods
On Fri, 22 Jun 2018, Geert Uytterhoeven wrote: > On Fri, Jun 22, 2018 at 2:49 PM Thomas Gleixner wrote: > > On Fri, 22 Jun 2018, Geert Uytterhoeven wrote: > > > For the common cases where 1000 is a multiple of HZ, or HZ is a multiple > > > of 1000, jiffies_to_msecs() never returns zero when passed a non-zero > > > time period. > > > > > > However, if HZ > 1000 and not an integer multiple of 1000 (e.g. 1024 or > > > 1200, as used on alpha and DECstation), jiffies_to_msecs() may return > > > zero for small non-zero time periods. This may break code that relies > > > on receiving back a non-zero value. > > > > > > jiffies_to_usecs() does not need such a fix, as does > > > not support values of HZ larger than 12287, thus rejecting any > > > problematic huge values of HZ. > > > > Sorry, I'm not understanding that sentence at all. > > Sorry for being unclear. > > 1 jiffy can only be less than 1µs if HZ > 100. > But include/linux/jiffies.h checks if HZ >= 12288, and does #error otherwise. > In addition, there's a "BUILD_BUG_ON(HZ > USEC_PER_SEC)" in time.c Hmm, ok. Care to reword? > > > Signed-off-by: Geert Uytterhoeven > > > > This lacks a stable tag, right? > > Up to the maintainer to add, isn't it? Yes and no. At least a hint that this has been broken by commit X or has been broken forever would be appreciated. Thanks, tglx
Re: [PATCH 3/4] PCI: Remove unused declarations
On Thu, 5 Oct 2017, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelg...@google.com> > > Remove these unused declarations: > > pcibios_config_init() # never defined anywhere > pcibios_scan_root()# only defined by x86 > pcibios_get_irq_routing_table()# only defined by x86 > pcibios_set_irq_routing() # only defined by x86 > > Signed-off-by: Bjorn Helgaas <bhelg...@google.com> Reviewed-by: Thomas Gleixner <t...@linutronix.de> -- To unsubscribe from this list: send the line "unsubscribe linux-alpha" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] futex: remove duplicated code and fix UB
On Thu, 24 Aug 2017, Will Deacon wrote:
> On Thu, Aug 24, 2017 at 09:31:05AM +0200, Jiri Slaby wrote:
> > +static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user
> > *uaddr)
> > +{
> > + unsigned int op = (encoded_op & 0x7000) >> 28;
> > + unsigned int cmp =(encoded_op & 0x0f00) >> 24;
> > + int oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 12);
> > + int cmparg = sign_extend32(encoded_op & 0x0fff, 12);
> > + int oldval, ret;
> > +
> > + if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
> > + if (oparg < 0 || oparg > 31)
> > + return -EINVAL;
> > + oparg = 1 << oparg;
> > + }
> > +
> > + if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
> > + return -EFAULT;
> > +
> > + ret = arch_futex_atomic_op_inuser(op, oparg, , uaddr);
> > + if (ret)
> > + return ret;
>
> We could move the pagefault_{disable,enable} calls here, and then remove
> them from the futex_atomic_op_inuser callsites elsewhere in futex.c
Correct, but we can do that after getting this in.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] futex: remove duplicated code and fix UB
On Mon, 26 Jun 2017, Jiri Slaby wrote:
> On 06/23/2017, 09:51 AM, Thomas Gleixner wrote:
> > On Wed, 21 Jun 2017, Jiri Slaby wrote:
> >> diff --git a/arch/arm64/include/asm/futex.h
> >> b/arch/arm64/include/asm/futex.h
> >> index f32b42e8725d..5bb2fd4674e7 100644
> >> --- a/arch/arm64/include/asm/futex.h
> >> +++ b/arch/arm64/include/asm/futex.h
> >> @@ -48,20 +48,10 @@ do {
> >> \
> >> } while (0)
> >>
> >> static inline int
> >> -futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
> >
> > That unsigned int seems to be a change from the arm64 tree in next. It's
> > not upstream and it'll cause a (easy to resolve) conflict.
>
> Ugh, I thought the arm64 is in upstream already. Note that this patch
> just takes what is in this arm64 fix and makes it effective for all
> architectures. So I will wait with v2 until it merges upstream.
Ok.
> > Yes, we probably can't change that anymore, but at least we should make it
> > very explicit and add a comment to that effect.
>
> Something like this or do you want a comment yet?
> unsigned int op = (encoded_op & 0x7000) >> 28;
> unsigned int cmp =(encoded_op & 0x0f00) >> 24;
> int oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 12);
> int cmparg = sign_extend32(encoded_op & 0x0fff, 12);
Yes, that makes sense.
There is also the issue with the shift. See this thread for further
reference:
http://lkml.kernel.org/r/alpine.DEB.2.20.1706282353190.1890@nanos
The gist is:
"Anything using a shift value < 0 or > 31 will get crap as a
result. Rightfully so because it's just undefined.
Yes I know that the insanity of user space is unlimited, but anything
attempting this is so broken that we cannot break it further by making
that shift arg unsigned and actually limit it to 0-31"
So we should make that case explicit as well.
if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
if (oparg < 0 || oparg > 31)
return -EINVAL;
oparg = 1 << oparg;
}
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] futex: remove duplicated code and fix UB
On Wed, 21 Jun 2017, Jiri Slaby wrote:
> diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
> index f32b42e8725d..5bb2fd4674e7 100644
> --- a/arch/arm64/include/asm/futex.h
> +++ b/arch/arm64/include/asm/futex.h
> @@ -48,20 +48,10 @@ do {
> \
> } while (0)
>
> static inline int
> -futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
That unsigned int seems to be a change from the arm64 tree in next. It's
not upstream and it'll cause a (easy to resolve) conflict.
> +static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
> +{
> + int op = (encoded_op >> 28) & 7;
> + int cmp = (encoded_op >> 24) & 15;
> + int oparg = (int)(encoded_op << 8) >> 20;
> + int cmparg = (int)(encoded_op << 20) >> 20;
So this is really bad. We have implicit and explicit type casting to
int. And while we are at it can we please stop proliferating the existing
mess.
'op' and 'cmp' definitly can be unsigned int. There is no reason to cast
them to int.
oparg, cmparg and oldval are more interesting.
The logic here is "documented" in uapi/linux/futex.h
/* FUTEX_WAKE_OP will perform atomically
int oldval = *(int *)UADDR2;
*(int *)UADDR2 = oldval OP OPARG;
if (oldval CMP CMPARG)
wake UADDR2; */
Now the FUTEX_OP macro which is supposed to compose the encoded_up does:
#define FUTEX_OP(op, oparg, cmp, cmparg) \
(((op & 0xf) << 28) | ((cmp & 0xf) << 24) \
| ((oparg & 0xfff) << 12) | (cmparg & 0xfff))
Of course this all is not typed, undocumented and completely ill
defined.
> + int oparg = (int)(encoded_op << 8) >> 20;
> + int cmparg = (int)(encoded_op << 20) >> 20;
So in fact we sign expand the 12 bits of oparg and cmparg. Really
intuitive.
Yes, we probably can't change that anymore, but at least we should make it
very explicit and add a comment to that effect.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] futex: remove duplicated code
On Thu, 25 May 2017, Will Deacon wrote:
> On Mon, May 22, 2017 at 11:11:33PM +0200, Thomas Gleixner wrote:
> > On Mon, 15 May 2017, Will Deacon wrote:
> > > On Mon, May 15, 2017 at 03:07:42PM +0200, Jiri Slaby wrote:
> > > > There is code duplicated over all architecture's headers for
> > > > futex_atomic_op_inuser. Namely op decoding, access_ok check for uaddr,
> > > > and comparison of the result.
> > > >
> > > > Remove this duplication and leave up to the arches only the needed
> > > > assembly which is now in arch_futex_atomic_op_inuser.
> > > >
> > > > Note that s390 removed access_ok check in d12a29703 ("s390/uaccess:
> > > > remove pointless access_ok() checks") as access_ok there returns true.
> > > > We introduce it back to the helper for the sake of simplicity (it gets
> > > > optimized away anyway).
> > >
> > > Whilst I think this is a good idea, the code in question actually results
> > > in undefined behaviour per the C spec and is reported by UBSAN. See my
> > > patch fixing arm64 here (which I'd forgotten about):
> > >
> > > https://www.spinics.net/lists/linux-arch/msg38564.html
> > >
> > > But, as stated in the thread above, I think we should go a step further
> > > and remove FUTEX_OP_{OR,ANDN,XOR,OPARG_SHIFT} altogether. They don't
> > > appear to be used by userspace, and this whole thing is a total mess.
> >
> > You wish. The constants are not used, but FUTEX_WAKE_OP _IS_ used by
> > glibc. They only have one argument it seems:
> >
> >#define FUTEX_OP_CLEAR_WAKE_IF_GT_ONE ((4 << 24) | 1)
> >
> > but I'm pretty sure that there is enough (probably horrible) code (think
> > java) out there using FUTEX_WAKE_OP for whatever (non)sensical reasons in
> > any available combination.
>
> Indeed, and I'm not proposing to get rid of that. It's the grossly
> over-engineered array of operations and the FUTEX_OP_OPARG_SHIFT modifier
> that I think we should kill. The latter likely behaves differently across
> different architectures and potentially depending on the toolchain you used
> to build the kernel.
>
> Does anybody know the history behind the interface design?
Which design?
4732efbeb997 ("[PATCH] FUTEX_WAKE_OP: pthread_cond_signal() speedup")
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] futex: remove duplicated code
On Mon, 15 May 2017, Will Deacon wrote:
> Hi Jiri,
>
> On Mon, May 15, 2017 at 03:07:42PM +0200, Jiri Slaby wrote:
> > There is code duplicated over all architecture's headers for
> > futex_atomic_op_inuser. Namely op decoding, access_ok check for uaddr,
> > and comparison of the result.
> >
> > Remove this duplication and leave up to the arches only the needed
> > assembly which is now in arch_futex_atomic_op_inuser.
> >
> > Note that s390 removed access_ok check in d12a29703 ("s390/uaccess:
> > remove pointless access_ok() checks") as access_ok there returns true.
> > We introduce it back to the helper for the sake of simplicity (it gets
> > optimized away anyway).
>
> Whilst I think this is a good idea, the code in question actually results
> in undefined behaviour per the C spec and is reported by UBSAN. See my
> patch fixing arm64 here (which I'd forgotten about):
>
> https://www.spinics.net/lists/linux-arch/msg38564.html
>
> But, as stated in the thread above, I think we should go a step further
> and remove FUTEX_OP_{OR,ANDN,XOR,OPARG_SHIFT} altogether. They don't
> appear to be used by userspace, and this whole thing is a total mess.
You wish. The constants are not used, but FUTEX_WAKE_OP _IS_ used by
glibc. They only have one argument it seems:
#define FUTEX_OP_CLEAR_WAKE_IF_GT_ONE ((4 << 24) | 1)
but I'm pretty sure that there is enough (probably horrible) code (think
java) out there using FUTEX_WAKE_OP for whatever (non)sensical reasons in
any available combination.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
