* [PATCH v17 2/5] random: add vgetrandom_alloc() syscall
From: Jason A. Donenfeld @ 2024-06-14 19:06 UTC (permalink / raw)
To: linux-kernel, patches, tglx
Cc: Jason A. Donenfeld, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
In-Reply-To: <20240614190646.2081057-1-Jason@zx2c4.com>
The vDSO getrandom() works over an opaque per-thread state of an
unexported size, which must be marked VM_WIPEONFORK, VM_DONTDUMP,
VM_NORESERVE, and VM_DROPPABLE for proper operation. Over time, the
nuances of these allocations may change or grow or even differ based on
architectural features.
The syscall has the signature:
void *vgetrandom_alloc(unsigned int *num, unsigned int *size_per_each,
unsigned long addr, unsigned int flags);
This takes a hinted number of opaque states in `num`, and returns a
pointer to an array of opaque states, the number actually allocated back
in `num`, and the size in bytes of each one in `size_per_each`, enabling
a libc to slice up the returned array into a state per each thread,
while ensuring that no single state straddles a page boundary. (The
`flags` and `addr` arguments, as well as the `*size_per_each` input
value, are reserved for the future and are forced to be zero zero for
now.)
Libc is expected to allocate a chunk of these on first use, and then
dole them out to threads as they're created, allocating more when
needed. The returned address of the first state may be passed to
munmap(2) with a length of `DIV_ROUND_UP(num, PAGE_SIZE / size_per_each)
* PAGE_SIZE`, in order to deallocate the memory.
We very intentionally do *not* leave state allocation for vDSO
getrandom() up to userspace itself, but rather provide this new syscall
for such allocations. vDSO getrandom() must not store its state in just
any old memory address, but rather just ones that the kernel specially
allocates for it, leaving the particularities of those allocations up to
the kernel.
The allocation of states is intended to be integrated into libc's thread
management. As an illustrative example, the following code might be used
to do the same outside of libc. Though, vgetrandom_alloc() is not
expected to be exposed outside of libc, and the pthread usage here is
expected to be elided into libc internals. This allocation scheme is
very naive and does not shrink; other implementations may choose to be
more complex.
static void *vgetrandom_alloc(unsigned int *num, unsigned int *size_per_each)
{
*size_per_each = 0; /* Must be zero on input. */
return (void *)syscall(__NR_vgetrandom_alloc, &num, &size_per_each,
0 /* reserved @addr */, 0 /* reserved @flags */);
}
static struct {
pthread_mutex_t lock;
void **states;
size_t len, cap;
} grnd_allocator = {
.lock = PTHREAD_MUTEX_INITIALIZER
};
static void *vgetrandom_get_state(void)
{
void *state = NULL;
pthread_mutex_lock(&grnd_allocator.lock);
if (!grnd_allocator.len) {
size_t new_cap;
size_t page_size = getpagesize();
unsigned int num = sysconf(_SC_NPROCESSORS_ONLN); /* Could be arbitrary, just a hint. */
unsigned int size_per_each;
void *new_block = vgetrandom_alloc(&num, &size_per_each);
void *new_states;
if (new_block == MAP_FAILED)
goto out;
new_cap = grnd_allocator.cap + num;
new_states = reallocarray(grnd_allocator.states, new_cap, sizeof(*grnd_allocator.states));
if (!new_states) {
munmap(new_block, DIV_ROUND_UP(num, page_size / size_per_each) * page_size);
goto out;
}
grnd_allocator.cap = new_cap;
grnd_allocator.states = new_states;
for (size_t i = 0; i < num; ++i) {
grnd_allocator.states[i] = new_block;
if (((uintptr_t)new_block & (page_size - 1)) + size_per_each > page_size)
new_block = (void *)(((uintptr_t)new_block + page_size) & (page_size - 1));
else
new_block += size_per_each;
}
grnd_allocator.len = num;
}
state = grnd_allocator.states[--grnd_allocator.len];
out:
pthread_mutex_unlock(&grnd_allocator.lock);
return state;
}
static void vgetrandom_put_state(void *state)
{
if (!state)
return;
pthread_mutex_lock(&grnd_allocator.lock);
grnd_allocator.states[grnd_allocator.len++] = state;
pthread_mutex_unlock(&grnd_allocator.lock);
}
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
MAINTAINERS | 1 +
drivers/char/random.c | 133 +++++++++++++++++++++++++++++++++++++++
include/linux/syscalls.h | 3 +
include/vdso/getrandom.h | 16 +++++
kernel/sys_ni.c | 3 +
lib/vdso/Kconfig | 6 ++
6 files changed, 162 insertions(+)
create mode 100644 include/vdso/getrandom.h
diff --git a/MAINTAINERS b/MAINTAINERS
index d6c90161c7bf..365de271415c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18749,6 +18749,7 @@ T: git https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git
F: Documentation/devicetree/bindings/rng/microsoft,vmgenid.yaml
F: drivers/char/random.c
F: drivers/virt/vmgenid.c
+F: include/vdso/getrandom.h
RAPIDIO SUBSYSTEM
M: Matt Porter <mporter@kernel.crashing.org>
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 2597cb43f438..2dd7329e76a4 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -8,6 +8,7 @@
* into roughly six sections, each with a section header:
*
* - Initialization and readiness waiting.
+ * - vDSO support helpers.
* - Fast key erasure RNG, the "crng".
* - Entropy accumulation and extraction routines.
* - Entropy collection routines.
@@ -39,6 +40,7 @@
#include <linux/blkdev.h>
#include <linux/interrupt.h>
#include <linux/mm.h>
+#include <linux/mman.h>
#include <linux/nodemask.h>
#include <linux/spinlock.h>
#include <linux/kthread.h>
@@ -56,6 +58,9 @@
#include <linux/sched/isolation.h>
#include <crypto/chacha.h>
#include <crypto/blake2s.h>
+#ifdef CONFIG_VDSO_GETRANDOM
+#include <vdso/getrandom.h>
+#endif
#include <asm/archrandom.h>
#include <asm/processor.h>
#include <asm/irq.h>
@@ -169,6 +174,134 @@ int __cold execute_with_initialized_rng(struct notifier_block *nb)
__func__, (void *)_RET_IP_, crng_init)
+
+/********************************************************************
+ *
+ * vDSO support helpers.
+ *
+ * The actual vDSO function is defined over in lib/vdso/getrandom.c,
+ * but this section contains the kernel-mode helpers to support that.
+ *
+ ********************************************************************/
+
+#ifdef CONFIG_VDSO_GETRANDOM
+/**
+ * sys_vgetrandom_alloc - Allocate opaque states for use with vDSO getrandom().
+ *
+ * @num: On input, a pointer to a suggested hint of how many states to
+ * allocate, and on return the number of states actually allocated.
+ *
+ * @size_per_each: On input, must be zero. On return, the size of each state allocated,
+ * so that the caller can split up the returned allocation into
+ * individual states.
+ *
+ * @addr: Reserved, must be zero.
+ *
+ * @flags: Reserved, must be zero.
+ *
+ * The getrandom() vDSO function in userspace requires an opaque state, which
+ * this function allocates by mapping a certain number of special pages into
+ * the calling process. It takes a hint as to the number of opaque states
+ * desired, and provides the caller with the number of opaque states actually
+ * allocated, the size of each one in bytes, and the address of the first
+ * state, which may be split up into @num states of @size_per_each bytes each,
+ * by adding @size_per_each to the returned first state @num times, while
+ * ensuring that no single state straddles a page boundary.
+ *
+ * Returns the address of the first state in the allocation on success, or a
+ * negative error value on failure.
+ *
+ * The returned address of the first state may be passed to munmap(2) with a
+ * length of `DIV_ROUND_UP(num, PAGE_SIZE / size_per_each) * PAGE_SIZE`, in
+ * order to deallocate the memory, after which it is invalid to pass it to vDSO
+ * getrandom().
+ *
+ * States allocated by this function must not be dereferenced, written, read,
+ * or otherwise manipulated. The *only* supported operations are:
+ * - Splitting up the states in intervals of @size_per_each, no more than
+ * @num times from the first state, while ensuring that no single state
+ * straddles a page boundary.
+ * - Passing a state to the getrandom() vDSO function's @opaque_state
+ * parameter, but not passing the same state at the same time to two such
+ * calls.
+ * - Passing the first state and the total length to munmap(2), as described
+ * above.
+ * All other uses are undefined behavior, which is subject to change or removal.
+ */
+SYSCALL_DEFINE4(vgetrandom_alloc, unsigned int __user *, num,
+ unsigned int __user *, size_per_each, unsigned long, addr,
+ unsigned int, flags)
+{
+ size_t state_size, alloc_size, num_states;
+ unsigned long pages_addr, populate;
+ unsigned int num_hint;
+ vm_flags_t vm_flags;
+ int ret;
+
+ /*
+ * @flags and @addr are currently unused, so in order to reserve them
+ * for the future, force them to be set to zero by current callers.
+ */
+ if (flags || addr)
+ return -EINVAL;
+
+ /*
+ * Also enforce that *size_per_each is zero on input, in case this becomes
+ * useful later on.
+ */
+ if (get_user(num_hint, size_per_each))
+ return -EFAULT;
+ if (num_hint)
+ return -EINVAL;
+
+ if (get_user(num_hint, num))
+ return -EFAULT;
+
+ state_size = sizeof(struct vgetrandom_state);
+ num_states = clamp_t(size_t, num_hint, 1, (SIZE_MAX & PAGE_MASK) / state_size);
+ alloc_size = PAGE_ALIGN(num_states * state_size);
+ /*
+ * States cannot straddle page boundaries, so calculate the number of
+ * states that can fit inside of a page without being split, and then
+ * multiply that out by the number of pages allocated.
+ */
+ num_states = (PAGE_SIZE / state_size) * (alloc_size / PAGE_SIZE);
+
+ vm_flags =
+ /*
+ * Don't allow state to be written to swap, to preserve forward secrecy.
+ * But also don't mlock it or pre-reserve it, and allow it to
+ * be discarded under memory pressure. If no memory is available, returns
+ * zeros rather than segfaulting.
+ */
+ VM_DROPPABLE | VM_NORESERVE |
+
+ /* Don't allow the state to survive forks, to prevent random number re-use. */
+ VM_WIPEONFORK |
+
+ /* Don't write random state into coredumps. */
+ VM_DONTDUMP;
+
+ if (mmap_write_lock_killable(current->mm))
+ return -EINTR;
+ pages_addr = do_mmap(NULL, 0, alloc_size, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, vm_flags, 0, &populate, NULL);
+ mmap_write_unlock(current->mm);
+ if (IS_ERR_VALUE(pages_addr))
+ return pages_addr;
+
+ ret = -EFAULT;
+ if (put_user(num_states, num) || put_user(state_size, size_per_each))
+ goto err_unmap;
+
+ return pages_addr;
+
+err_unmap:
+ vm_munmap(pages_addr, alloc_size);
+ return ret;
+}
+#endif
+
/*********************************************************************
*
* Fast key erasure RNG, the "crng".
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 9104952d323d..56368ea4f510 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -906,6 +906,9 @@ asmlinkage long sys_seccomp(unsigned int op, unsigned int flags,
void __user *uargs);
asmlinkage long sys_getrandom(char __user *buf, size_t count,
unsigned int flags);
+asmlinkage long sys_vgetrandom_alloc(unsigned int __user *num,
+ unsigned int __user *size_per_each,
+ unsigned long addr, unsigned int flags);
asmlinkage long sys_memfd_create(const char __user *uname_ptr, unsigned int flags);
asmlinkage long sys_bpf(int cmd, union bpf_attr *attr, unsigned int size);
asmlinkage long sys_execveat(int dfd, const char __user *filename,
diff --git a/include/vdso/getrandom.h b/include/vdso/getrandom.h
new file mode 100644
index 000000000000..e3ceb1976386
--- /dev/null
+++ b/include/vdso/getrandom.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ */
+
+#ifndef _VDSO_GETRANDOM_H
+#define _VDSO_GETRANDOM_H
+
+/**
+ * struct vgetrandom_state - State used by vDSO getrandom() and allocated by vgetrandom_alloc().
+ *
+ * Currently empty, as the vDSO getrandom() function has not yet been implemented.
+ */
+struct vgetrandom_state { int placeholder; };
+
+#endif /* _VDSO_GETRANDOM_H */
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index d7eee421d4bc..6b17fadb0f59 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -272,6 +272,9 @@ COND_SYSCALL(pkey_free);
/* memfd_secret */
COND_SYSCALL(memfd_secret);
+/* random */
+COND_SYSCALL(vgetrandom_alloc);
+
/*
* Architecture specific weak syscall entries.
*/
diff --git a/lib/vdso/Kconfig b/lib/vdso/Kconfig
index c46c2300517c..99661b731834 100644
--- a/lib/vdso/Kconfig
+++ b/lib/vdso/Kconfig
@@ -38,3 +38,9 @@ config GENERIC_VDSO_OVERFLOW_PROTECT
in the hotpath.
endif
+
+config VDSO_GETRANDOM
+ bool
+ select NEED_VM_DROPPABLE
+ help
+ Selected by architectures that support vDSO getrandom().
--
2.45.2
^ permalink raw reply related
* [PATCH v17 1/5] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: Jason A. Donenfeld @ 2024-06-14 19:06 UTC (permalink / raw)
To: linux-kernel, patches, tglx
Cc: Jason A. Donenfeld, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand, linux-mm
In-Reply-To: <20240614190646.2081057-1-Jason@zx2c4.com>
The vDSO getrandom() implementation works with a buffer allocated with a
new system call that has certain requirements:
- It shouldn't be written to core dumps.
* Easy: VM_DONTDUMP.
- It should be zeroed on fork.
* Easy: VM_WIPEONFORK.
- It shouldn't be written to swap.
* Uh-oh: mlock is rlimited.
* Uh-oh: mlock isn't inherited by forks.
It turns out that the vDSO getrandom() function has three really nice
characteristics that we can exploit to solve this problem:
1) Due to being wiped during fork(), the vDSO code is already robust to
having the contents of the pages it reads zeroed out midway through
the function's execution.
2) In the absolute worst case of whatever contingency we're coding for,
we have the option to fallback to the getrandom() syscall, and
everything is fine.
3) The buffers the function uses are only ever useful for a maximum of
60 seconds -- a sort of cache, rather than a long term allocation.
These characteristics mean that we can introduce VM_DROPPABLE, which
has the following semantics:
a) It never is written out to swap.
b) Under memory pressure, mm can just drop the pages (so that they're
zero when read back again).
c) It is inherited by fork.
d) It doesn't count against the mlock budget, since nothing is locked.
This is fairly simple to implement, with the one snag that we have to
use 64-bit VM_* flags, but this shouldn't be a problem, since the only
consumers will probably be 64-bit anyway.
This way, allocations used by vDSO getrandom() can use:
VM_DROPPABLE | VM_DONTDUMP | VM_WIPEONFORK | VM_NORESERVE
And there will be no problem with using memory when not in use, not
wiping on fork(), coredumps, or writing out to swap.
Cc: linux-mm@kvack.org
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
fs/proc/task_mmu.c | 3 +++
include/linux/mm.h | 8 ++++++++
include/trace/events/mmflags.h | 7 +++++++
mm/Kconfig | 3 +++
mm/mprotect.c | 2 +-
mm/rmap.c | 8 +++++---
6 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index f8d35f993fe5..1883d6462ca8 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -706,6 +706,9 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
#endif /* CONFIG_HAVE_ARCH_USERFAULTFD_MINOR */
#ifdef CONFIG_X86_USER_SHADOW_STACK
[ilog2(VM_SHADOW_STACK)] = "ss",
+#endif
+#ifdef CONFIG_NEED_VM_DROPPABLE
+ [ilog2(VM_DROPPABLE)] = "dp",
#endif
};
size_t i;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9849dfda44d4..5978cb4cc21c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -321,12 +321,14 @@ extern unsigned int kobjsize(const void *objp);
#define VM_HIGH_ARCH_BIT_3 35 /* bit only usable on 64-bit architectures */
#define VM_HIGH_ARCH_BIT_4 36 /* bit only usable on 64-bit architectures */
#define VM_HIGH_ARCH_BIT_5 37 /* bit only usable on 64-bit architectures */
+#define VM_HIGH_ARCH_BIT_6 38 /* bit only usable on 64-bit architectures */
#define VM_HIGH_ARCH_0 BIT(VM_HIGH_ARCH_BIT_0)
#define VM_HIGH_ARCH_1 BIT(VM_HIGH_ARCH_BIT_1)
#define VM_HIGH_ARCH_2 BIT(VM_HIGH_ARCH_BIT_2)
#define VM_HIGH_ARCH_3 BIT(VM_HIGH_ARCH_BIT_3)
#define VM_HIGH_ARCH_4 BIT(VM_HIGH_ARCH_BIT_4)
#define VM_HIGH_ARCH_5 BIT(VM_HIGH_ARCH_BIT_5)
+#define VM_HIGH_ARCH_6 BIT(VM_HIGH_ARCH_BIT_6)
#endif /* CONFIG_ARCH_USES_HIGH_VMA_FLAGS */
#ifdef CONFIG_ARCH_HAS_PKEYS
@@ -357,6 +359,12 @@ extern unsigned int kobjsize(const void *objp);
# define VM_SHADOW_STACK VM_NONE
#endif
+#ifdef CONFIG_NEED_VM_DROPPABLE
+# define VM_DROPPABLE VM_HIGH_ARCH_6
+#else
+# define VM_DROPPABLE VM_NONE
+#endif
+
#if defined(CONFIG_X86)
# define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
#elif defined(CONFIG_PPC)
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index e46d6e82765e..fab7848df50a 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -165,6 +165,12 @@ IF_HAVE_PG_ARCH_X(arch_3)
# define IF_HAVE_UFFD_MINOR(flag, name)
#endif
+#ifdef CONFIG_NEED_VM_DROPPABLE
+# define IF_HAVE_VM_DROPPABLE(flag, name) {flag, name},
+#else
+# define IF_HAVE_VM_DROPPABLE(flag, name)
+#endif
+
#define __def_vmaflag_names \
{VM_READ, "read" }, \
{VM_WRITE, "write" }, \
@@ -197,6 +203,7 @@ IF_HAVE_VM_SOFTDIRTY(VM_SOFTDIRTY, "softdirty" ) \
{VM_MIXEDMAP, "mixedmap" }, \
{VM_HUGEPAGE, "hugepage" }, \
{VM_NOHUGEPAGE, "nohugepage" }, \
+IF_HAVE_VM_DROPPABLE(VM_DROPPABLE, "droppable" ) \
{VM_MERGEABLE, "mergeable" } \
#define show_vma_flags(flags) \
diff --git a/mm/Kconfig b/mm/Kconfig
index b4cb45255a54..6cd65ea4b3ad 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -1056,6 +1056,9 @@ config ARCH_USES_HIGH_VMA_FLAGS
bool
config ARCH_HAS_PKEYS
bool
+config NEED_VM_DROPPABLE
+ select ARCH_USES_HIGH_VMA_FLAGS
+ bool
config ARCH_USES_PG_ARCH_X
bool
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 8c6cd8825273..57b8dad9adcc 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -623,7 +623,7 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
may_expand_vm(mm, oldflags, nrpages))
return -ENOMEM;
if (!(oldflags & (VM_ACCOUNT|VM_WRITE|VM_HUGETLB|
- VM_SHARED|VM_NORESERVE))) {
+ VM_SHARED|VM_NORESERVE|VM_DROPPABLE))) {
charged = nrpages;
if (security_vm_enough_memory_mm(mm, charged))
return -ENOMEM;
diff --git a/mm/rmap.c b/mm/rmap.c
index e8fc5ecb59b2..d873a3f06506 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1397,7 +1397,8 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
VM_WARN_ON_FOLIO(folio_test_hugetlb(folio), folio);
VM_BUG_ON_VMA(address < vma->vm_start ||
address + (nr << PAGE_SHIFT) > vma->vm_end, vma);
- __folio_set_swapbacked(folio);
+ if (!(vma->vm_flags & VM_DROPPABLE))
+ __folio_set_swapbacked(folio);
__folio_set_anon(folio, vma, address, true);
if (likely(!folio_test_large(folio))) {
@@ -1841,7 +1842,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
* plus the rmap(s) (dropped by discard:).
*/
if (ref_count == 1 + map_count &&
- !folio_test_dirty(folio)) {
+ (!folio_test_dirty(folio) || (vma->vm_flags & VM_DROPPABLE))) {
dec_mm_counter(mm, MM_ANONPAGES);
goto discard;
}
@@ -1851,7 +1852,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
* discarded. Remap the page to page table.
*/
set_pte_at(mm, address, pvmw.pte, pteval);
- folio_set_swapbacked(folio);
+ if (!(vma->vm_flags & VM_DROPPABLE))
+ folio_set_swapbacked(folio);
ret = false;
page_vma_mapped_walk_done(&pvmw);
break;
--
2.45.2
^ permalink raw reply related
* [PATCH v17 0/5] implement getrandom() in vDSO
From: Jason A. Donenfeld @ 2024-06-14 19:06 UTC (permalink / raw)
To: linux-kernel, patches, tglx
Cc: Jason A. Donenfeld, linux-crypto, linux-api, x86,
Greg Kroah-Hartman, Adhemerval Zanella Netto, Carlos O'Donell,
Florian Weimer, Arnd Bergmann, Jann Horn, Christian Brauner,
David Hildenbrand
Changes v14->{present}:
----------------------
This is back after a bit of a hiatus. In the last attempt to do this in
the beginning of 2023, I think we reached consensus on a few things --
the use case, the vDSO implementation and semantics, its integration
with libc, the test code and documentation, and so forth. It was
basically "ready to go". Almost. But there was a lingering issue that
bogged this down, which is that it demanded some new mm semantics that
weren't very popular.
In particular, the series from last year made use of the x86 instruction
decoder to just skip over faulting instructions. I still think this is
nifty, but it's not actually essential for the semantics needed, and I
can understand why this was by far the largest objection. So all of that
is dropped, which simplifies quite a bit.
In another avenue of the mm discussion, Andy had mentioned using
_install_special_mapping() instead of the VM_DROPPABLE work, and I spent
a long while looking into this, and attempted several times to code up a
working implementation that used that. But the semantics really just
weren't possible without adding hooks to lots of other core code, and
duplicating a lot of code that really ought not to be. So I've kept the
VM_DROPPABLE patch here, but because the x86 instruction decoding stuff
has been removed, that patch is actually a lot smaller and simpler and I
don't think should be too controversial. In terms of actual C code, it
only adds around 6 lines, and is compact enough that you can just grep
for VM_DROPPABLE to see the whole thing.
The original cover letter is produced below. I'm eager to finally get
this patchset moving, and sorry for the delay in producing the v+1 from
before.
Assuming this goes well, the plan would be to take this through my
random.git tree for 6.11. And if the mm part looks fine, I'll get this
cooking in linux-next ASAP.
Thanks ahead of time for taking a look at it.
Changes v16->v17:
- Generate patchset using --base.
- Rebase on 6.10-rc1, which means bumping the syscall number to 463.
- Adjust documentation and example code to specify proper munmap() deallocation
calculation.
- Use %xmm0 instead of %xmm8 as temp register for more compact encoding.
- Documentation comment syntax fixups.
- If page-straddling address is passed to vgetrandom, rather than falling back
to the syscall, return -EFAULT.
- Get rid of vdso_kernel_ulong type and just use u64 uniformly.
- Don't include crypto/chacha.h inside of a vdso header, to keep kernel code
out of the vdso user code.
- Balance christmas trees.
- Improve comments of vgetrandom() function.
- Get rid of NOWARN|NORETRY memory allocation logic, and also don't clear the
OOM flag on failure. This is kind of a problem, but Jann and Michal thought
that it was better to crash than for userspace to keep retrying instructions.
And the pre-v14 series that tried to skip the instruction instead of retrying
was marred by controversy. So just table this for later; it's probably not
essential now.
Changes v15->v16:
- DavidH pointed out a missing swap edge case in 1/5.
- Mostly just a resend because I forgot --cc-cover, and sent it during
the merge window.
--------------
Useful links:
- This series: https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/log/?h=vdso
- Glibc patches by Adhemerval against glibc-2.39: https://git.zx2c4.com/glibc/log/?h=vdso
- You may also want these if you're daily driving this as I am on a normal
desktop system:
- systemd patch: https://github.com/systemd/systemd/pull/25519
- libseccomp patch: https://github.com/seccomp/libseccomp/pull/395
- In case you're actually interested in the v≤14 design where faults were
non-fatal and instructions were skipped (which I think is more coherent, even
if the implementation is controversial), this lives in my branch here:
https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/log/?h=jd/vdso-skip-insn
Note that I'm *not* actually proposing this for upstream at this time. But it
may be of conversational interest.
-------------
Two statements:
1) Userspace wants faster cryptographically secure random numbers of
arbitrary size, big or small.
2) Userspace is currently unable to safely roll its own RNG with the
same security profile as getrandom().
Statement (1) has been debated for years, with arguments ranging from
"we need faster cryptographically secure card shuffling!" to "the only
things that actually need good randomness are keys, which are few and
far between" to "actually, TLS CBC nonces are frequent" and so on. I
don't intend to wade into that debate substantially, except to note that
recently glibc added arc4random(), whose goal is to return a
cryptographically secure uint32_t, and there are real user reports of it
being too slow. So here we are.
Statement (2) is more interesting. The kernel is the nexus of all
entropic inputs that influence the RNG. It is in the best position, and
probably the only position, to decide anything at all about the current
state of the RNG and of its entropy. One of the things it uniquely knows
about is when reseeding is necessary.
For example, when a virtual machine is forked, restored, or duplicated,
it's imparative that the RNG doesn't generate the same outputs. For this
reason, there's a small protocol between hypervisors and the kernel that
indicates this has happened, alongside some ID, which the RNG uses to
immediately reseed, so as not to return the same numbers. Were userspace
to expand a getrandom() seed from time T1 for the next hour, and at some
point T2 < hour, the virtual machine forked, userspace would continue to
provide the same numbers to two (or more) different virtual machines,
resulting in potential cryptographic catastrophe. Something similar
happens on resuming from hibernation (or even suspend), with various
compromise scenarios there in mind.
There's a more general reason why userspace rolling its own RNG from a
getrandom() seed is fraught. There's a lot of attention paid to this
particular Linuxism we have of the RNG being initialized and thus
non-blocking or uninitialized and thus blocking until it is initialized.
These are our Two Big States that many hold to be the holy
differentiating factor between safe and not safe, between
cryptographically secure and garbage. The fact is, however, that the
distinction between these two states is a hand-wavy wishy-washy inexact
approximation. Outside of a few exceptional cases (e.g. a HW RNG is
available), we actually don't really ever know with any rigor at all
when the RNG is safe and ready (nor when it's compromised). We do the
best we can to "estimate" it, but entropy estimation is fundamentally
impossible in the general case. So really, we're just doing guess work,
and hoping it's good and conservative enough. Let's then assume that
there's always some potential error involved in this differentiator.
In fact, under the surface, the RNG is engineered around a different
principal, and that is trying to *use* new entropic inputs regularly and
at the right specific moments in time. For example, close to boot time,
the RNG reseeds itself more often than later. At certain events, like VM
fork, the RNG reseeds itself immediately. The various heuristics for
when the RNG will use new entropy and how often is really a core aspect
of what the RNG has some potential to do decently enough (and something
that will probably continue to improve in the future from random.c's
present set of algorithms). So in your mind, put away the metal
attachment to the Two Big States, which represent an approximation with
a potential margin of error. Instead keep in mind that the RNG's primary
operating heuristic is how often and exactly when it's going to reseed.
So, if userspace takes a seed from getrandom() at point T1, and uses it
for the next hour (or N megabytes or some other meaningless metric),
during that time, potential errors in the Two Big States approximation
are amplified. During that time potential reseeds are being lost,
forgotten, not reflected in the output stream. That's not good.
The simplest statement you could make is that userspace RNGs that expand
a getrandom() seed at some point T1 are nearly always *worse*, in some
way, than just calling getrandom() every time a random number is
desired.
For those reasons, after some discussion on libc-alpha, glibc's
arc4random() now just calls getrandom() on each invocation. That's
trivially safe, and gives us latitude to then make the safe thing faster
without becoming unsafe at our leasure. Card shuffling isn't
particularly fast, however.
How do we rectify this? By putting a safe implementation of getrandom()
in the vDSO, which has access to whatever information a
particular iteration of random.c is using to make its decisions. I use
that careful language of "particular iteration of random.c", because the
set of things that a vDSO getrandom() implementation might need for making
decisions as good as the kernel's will likely change over time. This
isn't just a matter of exporting certain *data* to userspace. We're not
going to commit to a "data API" where the various heuristics used are
exposed, locking in how the kernel works for decades to come, and then
leave it to various userspaces to roll something on top and shoot
themselves in the foot and have all sorts of complexity disasters.
Rather, vDSO getrandom() is supposed to be the *same exact algorithm*
that runs in the kernel, except it's been hoisted into userspace as
much as possible. And so vDSO getrandom() and kernel getrandom() will
always mirror each other hermetically.
API-wise, the vDSO gains this function:
ssize_t vgetrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state);
The return value and the first 3 arguments are the same as ordinary
getrandom(), while the last argument is a pointer to some state
allocated with vgetrandom_alloc(), explained below. Were all four
arguments passed to the getrandom syscall, nothing different would
happen, and the functions would have the exact same behavior.
Then, we introduce a new syscall:
void *vgetrandom_alloc(unsigned int *num, unsigned int *size_per_each,
unsigned long addr, unsigned int flags);
This takes a hinted number of opaque states in `num`, and returns a
pointer to an array of opaque states, the number actually allocated back
in `num`, and the size in bytes of each one in `size_per_each`, enabling
a libc to slice up the returned array into a state per each thread. (The
`flags` and `addr` arguments, as well as the `*size_per_each` input
value, are reserved for the future and are forced to be zero for now.)
Libc is expected to allocate a chunk of these on first use, and then
dole them out to threads as they're created, allocating more when
needed. The returned address of the first state may be passed to
munmap(2) with a length of `num * size_per_each`, in order to deallocate
the memory.
We very intentionally do *not* leave state allocation up to the caller
of vgetrandom, but provide vgetrandom_alloc for that allocation. There
are too many weird things that can go wrong, and it's important that
vDSO does not provide too generic of a mechanism. It's not going to
store its state in just any old memory address. It'll do it only in ones
it allocates.
Right now this means it uses a new mm flag called VM_DROPPABLE, along
with VM_WIPEONFORK. In the future maybe there will be other interesting
page flags or anti-heartbleed measures, or other platform-specific
kernel-specific things that can be set from the syscall. Again, it's
important that the kernel has a say in how this works rather than
agreeing to operate on any old address; memory isn't neutral.
The interesting meat of the implementation is in lib/vdso/getrandom.c,
as generic C code, and it aims to mainly follow random.c's buffered fast
key erasure logic. Before the RNG is initialized, it falls back to the
syscall. Right now it uses a simple generation counter to make its decisions
on reseeding (though this could be made more extensive over time).
The actual place that has the most work to do is in all of the other
files. Most of the vDSO shared page infrastructure is centered around
gettimeofday, and so the main structs are all in arrays for different
timestamp types, and attached to time namespaces, and so forth. I've
done the best I could to add onto this in an unintrusive way.
In my test results, performance is pretty stellar (around 15x for uint32_t
generation), and it seems to be working. There's an extended example in the
second commit of this series, showing how the syscall and the vDSO function
are meant to be used together.
Cc: linux-crypto@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: x86@kernel.org
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
Cc: Carlos O'Donell <carlos@redhat.com>
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jann Horn <jannh@google.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: David Hildenbrand <dhildenb@redhat.com>
Jason A. Donenfeld (5):
mm: add VM_DROPPABLE for designating always lazily freeable mappings
random: add vgetrandom_alloc() syscall
arch: allocate vgetrandom_alloc() syscall number
random: introduce generic vDSO getrandom() implementation
x86: vdso: Wire up getrandom() vDSO implementation
MAINTAINERS | 2 +
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd32.h | 2 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/Kconfig | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/x86/entry/vdso/Makefile | 3 +-
arch/x86/entry/vdso/vdso.lds.S | 2 +
arch/x86/entry/vdso/vgetrandom-chacha.S | 178 +++++++++++
arch/x86/entry/vdso/vgetrandom.c | 17 ++
arch/x86/include/asm/vdso/getrandom.h | 55 ++++
arch/x86/include/asm/vdso/vsyscall.h | 2 +
arch/x86/include/asm/vvar.h | 16 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
drivers/char/random.c | 144 +++++++++
fs/proc/task_mmu.c | 3 +
include/linux/mm.h | 8 +
include/linux/syscalls.h | 3 +
include/trace/events/mmflags.h | 7 +
include/uapi/asm-generic/unistd.h | 5 +-
include/vdso/datapage.h | 11 +
include/vdso/getrandom.h | 46 +++
kernel/sys_ni.c | 3 +
lib/vdso/Kconfig | 6 +
lib/vdso/getrandom.c | 228 ++++++++++++++
mm/Kconfig | 3 +
mm/mprotect.c | 2 +-
mm/rmap.c | 8 +-
tools/include/uapi/asm-generic/unistd.h | 5 +-
.../arch/mips/entry/syscalls/syscall_n64.tbl | 1 +
.../arch/powerpc/entry/syscalls/syscall.tbl | 1 +
.../perf/arch/s390/entry/syscalls/syscall.tbl | 1 +
.../arch/x86/entry/syscalls/syscall_64.tbl | 1 +
tools/testing/selftests/vDSO/.gitignore | 2 +
tools/testing/selftests/vDSO/Makefile | 15 +
.../testing/selftests/vDSO/vdso_test_chacha.c | 43 +++
.../selftests/vDSO/vdso_test_getrandom.c | 286 ++++++++++++++++++
48 files changed, 1118 insertions(+), 7 deletions(-)
create mode 100644 arch/x86/entry/vdso/vgetrandom-chacha.S
create mode 100644 arch/x86/entry/vdso/vgetrandom.c
create mode 100644 arch/x86/include/asm/vdso/getrandom.h
create mode 100644 include/vdso/getrandom.h
create mode 100644 lib/vdso/getrandom.c
create mode 100644 tools/testing/selftests/vDSO/vdso_test_chacha.c
create mode 100644 tools/testing/selftests/vDSO/vdso_test_getrandom.c
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
--
2.45.2
^ permalink raw reply
* Re: [PATCH v16 1/5] mm: add VM_DROPPABLE for designating always lazily freeable mappings
From: Jason A. Donenfeld @ 2024-06-14 18:35 UTC (permalink / raw)
To: Michal Hocko
Cc: Jann Horn, linux-kernel, patches, tglx, linux-crypto, linux-api,
x86, Greg Kroah-Hartman, Adhemerval Zanella Netto,
Carlos O'Donell, Florian Weimer, Arnd Bergmann,
Christian Brauner, David Hildenbrand, linux-mm
In-Reply-To: <Zmbq1dGPIYdRLw5_@tiehlicka>
On Mon, Jun 10, 2024 at 02:00:21PM +0200, Michal Hocko wrote:
> On Fri 07-06-24 17:50:34, Jann Horn wrote:
> [...]
> > Or, from a different angle: You're trying to allocate memory, and you
> > can't make forward progress until that memory has been allocated
> > (unless the process is killed). That's what GFP_KERNEL is for. Stuff
> > like "__GFP_NOWARN | __GFP_NORETRY" is for when you have a backup plan
> > that lets you make progress (perhaps in a slightly less efficient way,
> > or by dropping some incoming data, or something like that), and it
> > hints to the page allocator that it doesn't have to try hard to
> > reclaim memory if it can't find free memory quickly.
>
> Correct. A psedu-busy wait for allocation to succeed sounds like a very
> bad idea to imprint into ABI. Is there really any design requirement to
> make these mappings to never cause the OOM killer?
>
> Making the content dropable under memory pressure because it is
> inherently recoverable is something else (this is essentially an
> implicit MADV_FREE semantic) but putting a requirement on the memory
> allocation on the fault sounds just wrong to me.
The idea is that syscall getrandom() won't make a process be killed, so
neither should vgetrandom().
But there's an argument to be made that the NOWARN|NORETRY logic only
made sense with the now-dropped "skip instruction on fault" patch that
was so controversial before, since in that case, there wouldn't be
infinite retry, but rather skipping and then falling back to the
syscall. I think this is nicer behavior, but the implementation caused a
stir, so I'm not at the moment going that route. Given that, I think
I'll follow your advice and get rid of NOWARN|NORETRY for this too. And
then maybe we'll all revisit that later.
Jason
^ permalink raw reply
* Re: [PATCHv8 bpf-next 3/9] uprobe: Add uretprobe syscall to speed up return probe
From: Nathan Chancellor @ 2024-06-14 17:48 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta, Huacai Chen,
loongarch
In-Reply-To: <20240611112158.40795-4-jolsa@kernel.org>
Hi Jiri,
On Tue, Jun 11, 2024 at 01:21:52PM +0200, Jiri Olsa wrote:
> Adding uretprobe syscall instead of trap to speed up return probe.
...
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 2c83ba776fc7..2816e65729ac 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1474,11 +1474,20 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
> return ret;
> }
>
> +void * __weak arch_uprobe_trampoline(unsigned long *psize)
> +{
> + static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
This change as commit ff474a78cef5 ("uprobe: Add uretprobe syscall to
speed up return probe") in -next causes the following build error for
ARCH=loongarch allmodconfig:
In file included from include/linux/uprobes.h:49,
from include/linux/mm_types.h:16,
from include/linux/mmzone.h:22,
from include/linux/gfp.h:7,
from include/linux/xarray.h:16,
from include/linux/list_lru.h:14,
from include/linux/fs.h:13,
from include/linux/highmem.h:5,
from kernel/events/uprobes.c:13:
kernel/events/uprobes.c: In function 'arch_uprobe_trampoline':
arch/loongarch/include/asm/uprobes.h:12:33: error: initializer element is not constant
12 | #define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP)
| ^~~~~~~~~~~~~~~~~~~~
kernel/events/uprobes.c:1479:39: note: in expansion of macro 'UPROBE_SWBP_INSN'
1479 | static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
| ^~~~~~~~~~~~~~~~
> + *psize = UPROBE_SWBP_INSN_SIZE;
> + return &insn;
> +}
> +
> static struct xol_area *__create_xol_area(unsigned long vaddr)
> {
> struct mm_struct *mm = current->mm;
> - uprobe_opcode_t insn = UPROBE_SWBP_INSN;
> + unsigned long insns_size;
> struct xol_area *area;
> + void *insns;
>
> area = kmalloc(sizeof(*area), GFP_KERNEL);
> if (unlikely(!area))
> @@ -1502,7 +1511,8 @@ static struct xol_area *__create_xol_area(unsigned long vaddr)
> /* Reserve the 1st slot for get_trampoline_vaddr() */
> set_bit(0, area->bitmap);
> atomic_set(&area->slot_count, 1);
> - arch_uprobe_copy_ixol(area->pages[0], 0, &insn, UPROBE_SWBP_INSN_SIZE);
> + insns = arch_uprobe_trampoline(&insns_size);
> + arch_uprobe_copy_ixol(area->pages[0], 0, insns, insns_size);
>
> if (!xol_add_vma(mm, area))
> return area;
> @@ -1827,7 +1837,7 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
> *
> * Returns -1 in case the xol_area is not allocated.
> */
> -static unsigned long get_trampoline_vaddr(void)
> +unsigned long uprobe_get_trampoline_vaddr(void)
> {
> struct xol_area *area;
> unsigned long trampoline_vaddr = -1;
> @@ -1878,7 +1888,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> if (!ri)
> return;
>
> - trampoline_vaddr = get_trampoline_vaddr();
> + trampoline_vaddr = uprobe_get_trampoline_vaddr();
> orig_ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs);
> if (orig_ret_vaddr == -1)
> goto fail;
> @@ -2123,7 +2133,7 @@ static struct return_instance *find_next_ret_chain(struct return_instance *ri)
> return ri;
> }
>
> -static void handle_trampoline(struct pt_regs *regs)
> +void uprobe_handle_trampoline(struct pt_regs *regs)
> {
> struct uprobe_task *utask;
> struct return_instance *ri, *next;
> @@ -2187,8 +2197,8 @@ static void handle_swbp(struct pt_regs *regs)
> int is_swbp;
>
> bp_vaddr = uprobe_get_swbp_addr(regs);
> - if (bp_vaddr == get_trampoline_vaddr())
> - return handle_trampoline(regs);
> + if (bp_vaddr == uprobe_get_trampoline_vaddr())
> + return uprobe_handle_trampoline(regs);
>
> uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> if (!uprobe) {
> --
> 2.45.1
>
Cheers,
Nathan
^ permalink raw reply
* Re: termios constants should be unsigned
From: Alejandro Colomar @ 2024-06-13 21:37 UTC (permalink / raw)
To: Paul Eggert
Cc: Zack Weinberg, Andrew Morton, Palmer Dabbelt, linux-api,
GNU libc development, 'linux-man'
In-Reply-To: <6ce7434a-56bd-4e95-80f1-b2857834b0d4@cs.ucla.edu>
[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]
On Thu, Jun 13, 2024 at 02:12:20PM GMT, Paul Eggert wrote:
> Part of the issue here is that GCC and Clang often do a better job of
> warning when constants are signed, not unsigned. For example, suppose a
> program mistakenly packages termios flags along with three other bits into
> an 'unsigned long', with code like this:
>
> unsigned long
> tagged_pendin (unsigned tag)
> {
> return (PENDIN << 3) | tag;
> }
>
> Since PENDIN is 0x20000000 Clang and GCC by default warn about the mistake,
> as the signed integer overflow has undefined behavior. But if PENDIN were
> changed to 0x20000000U the behavior would be well-defined, there would be no
> warning even with -Wall -Wextra -Wsign-conversion, and the code would
> silently behave as if PENDIN were zero, which is not intended.
>
> This is another reason why appending "U" to PENDIN's value would have
> drawbacks as well as advantages.
Hmmmm, very interesting point! I'll have that in mind when doing
bitwise stuff with constants.
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: termios constants should be unsigned
From: Paul Eggert @ 2024-06-13 21:12 UTC (permalink / raw)
To: Zack Weinberg, Alejandro Colomar, Andrew Morton, Palmer Dabbelt,
linux-api, GNU libc development, 'linux-man'
In-Reply-To: <f6ee2bd3-c1b7-4769-a313-b62f42c450ca@app.fastmail.com>
On 6/13/24 05:32, Zack Weinberg wrote:
> there is still a need for
> caution around conversions that change signedness.
Yes, just as there is need for caution around any use of unsigned types.
Unfortunately in my experience Clang's (and even GCC's) warnings about
signedness conversion are more likely to cause harm than good, with this
thread being an example of the harm.
Part of the issue here is that GCC and Clang often do a better job of
warning when constants are signed, not unsigned. For example, suppose a
program mistakenly packages termios flags along with three other bits
into an 'unsigned long', with code like this:
unsigned long
tagged_pendin (unsigned tag)
{
return (PENDIN << 3) | tag;
}
Since PENDIN is 0x20000000 Clang and GCC by default warn about the
mistake, as the signed integer overflow has undefined behavior. But if
PENDIN were changed to 0x20000000U the behavior would be well-defined,
there would be no warning even with -Wall -Wextra -Wsign-conversion, and
the code would silently behave as if PENDIN were zero, which is not
intended.
This is another reason why appending "U" to PENDIN's value would have
drawbacks as well as advantages.
^ permalink raw reply
* Re: termios constants should be unsigned
From: Zack Weinberg @ 2024-06-13 12:32 UTC (permalink / raw)
To: Paul Eggert, Alejandro Colomar, Andrew Morton, Palmer Dabbelt,
linux-api, GNU libc development, 'linux-man'
In-Reply-To: <87af5e8f-0dcb-44a0-94de-757cad7d5ded@cs.ucla.edu>
On Wed, Jun 12, 2024, at 10:55 AM, Paul Eggert wrote:
> A lot of this stuff is pedanticism that dates back to the bad old days
> when the C standard allowed ones' complement and signed magnitude
> representations of signed integers. Although it can be amusing to
> worry about that possibility (I know I've done it) it's never been a
> practical worry, and even the motivation of pedanticism is going away
> now that C23 requires two's complement.
Unless C23 eliminated *all* the cases where an operation on unsigned
integers is well-defined but the same operation on signed integers is
undefined, and last I checked it had not, there is still a need for
caution around conversions that change signedness.
zw
^ permalink raw reply
* Re: termios constants should be unsigned
From: Alejandro Colomar @ 2024-06-12 22:22 UTC (permalink / raw)
To: enh
Cc: Paul Eggert, Andrew Morton, Palmer Dabbelt, linux-api, libc-alpha,
linux-man
In-Reply-To: <CAJgzZorzcAP5wNa-UCMyarmjgwVBveg0c0Dj36ByVEacnOHrnw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2293 bytes --]
Hi Elliott,
On Wed, Jun 12, 2024 at 05:54:43PM GMT, enh wrote:
> > BTW, that seems to be a bogus way to workaround this; the cast should
> > have been on the other side. I'd say whoever maintains that code should
> > probably fix that to use unsigned types.
>
> indeed. i've already sent out such a change :-)
>
> > These constants are meant to
> > be 'tcflag_t', so a cast should be to that type, or the type of the
> > other side of the comparison, but casting to 'int' just for silencing a
> > waring seems nuts.
>
> i suspect the reasoning was one of readability --- keeping the [short]
> constants legible at the cost of making the expression slightly
> longer.
>
> > This makes me wonder if breaking _those_ users could be a good thing...
>
> like Paul Eggert said somewhere else today --- only if we're finding
> real bugs. and so far we're not.
>
> it's like the warn_unused_result argument. a purist would argue that
> every function should have that annotation, because you should always
> check for errors, and if you're not already doing so, your code is
> already broken. whereas a pragmatist would argue that most people are
> just going to add the "shut up, compiler" cast (or disable the warning
> entirely) if their already-working code suddenly starts spamming
> warnings next time they build it.
>
> while my bar for that might not be as high as my bar for ABI breakage,
> my source compatibility bar is still pretty high. it would be almost
> unethical of me to make app developers do random busywork. i have to
> be pretty confident (as with, say, "you just passed an fd > 1024 to an
> fd_set function/macro and thus corrupted memory") that their code is
> _definitely_ wrong. (and even there, that's going to have to be a
> runtime check!)
Yeah, I can agree with that. I'm that kind of pedantic purist for my
own code, and it's painful that historic accidents like this one don't
allow me to be so in my own code. But I agree that fixing the entire
world when their code is braindamaged but works is asking too much.
I'll just disable that pedantic warning when I use termbits. :)
Btw, thanks for fixing that brain-damaged cast. ;-)
Have a lovely night!
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: termios constants should be unsigned
From: enh @ 2024-06-12 21:54 UTC (permalink / raw)
To: Alejandro Colomar
Cc: Paul Eggert, Andrew Morton, Palmer Dabbelt, linux-api, libc-alpha,
linux-man
In-Reply-To: <5rfohnr4rs3tkfs7y3f7rth36c67pvcwv4q52onrjohdjtpo7m@stvcsncq7z4f>
On Wed, Jun 12, 2024 at 3:01 PM Alejandro Colomar <alx@kernel.org> wrote:
>
> On Wed, Jun 12, 2024 at 01:47:03PM GMT, enh wrote:
> > hacked these changes into AOSP, and it did break one bit of existing
> > code that was already working around the sign differences --- this
> > warning was enabled but the code had a cast to make the _other_ side
> > of the comparison signed (rather than make this side of the comparison
> > unsigned).
>
> BTW, that seems to be a bogus way to workaround this; the cast should
> have been on the other side. I'd say whoever maintains that code should
> probably fix that to use unsigned types.
indeed. i've already sent out such a change :-)
> These constants are meant to
> be 'tcflag_t', so a cast should be to that type, or the type of the
> other side of the comparison, but casting to 'int' just for silencing a
> waring seems nuts.
i suspect the reasoning was one of readability --- keeping the [short]
constants legible at the cost of making the expression slightly
longer.
> This makes me wonder if breaking _those_ users could be a good thing...
like Paul Eggert said somewhere else today --- only if we're finding
real bugs. and so far we're not.
it's like the warn_unused_result argument. a purist would argue that
every function should have that annotation, because you should always
check for errors, and if you're not already doing so, your code is
already broken. whereas a pragmatist would argue that most people are
just going to add the "shut up, compiler" cast (or disable the warning
entirely) if their already-working code suddenly starts spamming
warnings next time they build it.
while my bar for that might not be as high as my bar for ABI breakage,
my source compatibility bar is still pretty high. it would be almost
unethical of me to make app developers do random busywork. i have to
be pretty confident (as with, say, "you just passed an fd > 1024 to an
fd_set function/macro and thus corrupted memory") that their code is
_definitely_ wrong. (and even there, that's going to have to be a
runtime check!)
> --
> <https://www.alejandro-colomar.es/>
^ permalink raw reply
* Re: termios constants should be unsigned
From: Alejandro Colomar @ 2024-06-12 19:01 UTC (permalink / raw)
To: enh
Cc: Paul Eggert, Andrew Morton, Palmer Dabbelt, linux-api, libc-alpha,
linux-man
In-Reply-To: <CAJgzZorNc3gNVbiibz+DibrMLxc2dQoOS5NtL+RQUkSD-GMYaA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 854 bytes --]
On Wed, Jun 12, 2024 at 01:47:03PM GMT, enh wrote:
> hacked these changes into AOSP, and it did break one bit of existing
> code that was already working around the sign differences --- this
> warning was enabled but the code had a cast to make the _other_ side
> of the comparison signed (rather than make this side of the comparison
> unsigned).
BTW, that seems to be a bogus way to workaround this; the cast should
have been on the other side. I'd say whoever maintains that code should
probably fix that to use unsigned types. These constants are meant to
be 'tcflag_t', so a cast should be to that type, or the type of the
other side of the comparison, but casting to 'int' just for silencing a
waring seems nuts.
This makes me wonder if breaking _those_ users could be a good thing...
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: termios constants should be unsigned
From: Alejandro Colomar @ 2024-06-12 18:55 UTC (permalink / raw)
To: enh
Cc: Paul Eggert, Andrew Morton, Palmer Dabbelt, linux-api, libc-alpha,
linux-man
In-Reply-To: <CAJgzZorNc3gNVbiibz+DibrMLxc2dQoOS5NtL+RQUkSD-GMYaA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]
Hi Elliott, Paul,
On Wed, Jun 12, 2024 at 01:47:03PM GMT, enh wrote:
> well, any change like this is a potential source incompatibility ... i
> hacked these changes into AOSP, and it did break one bit of existing
> code that was already working around the sign differences --- this
> warning was enabled but the code had a cast to make the _other_ side
> of the comparison signed (rather than make this side of the comparison
> unsigned).
>
> Android's libc [bionic] uses the uapi headers directly, so we would be
> affected, but to be clear --- i'm fine with this if the consensus is
> to go this way.
Hmmm; I see. I guess for this already-existent case we can just live
with it. Then I'll just ask to consider using unsigned constants for
new stuff that is a bit pattern and not just a number.
Let's drop the patch.
Have a lovely day!
Alex
> (but, yeah, i'm with the "how about we fix the language and compiler
> rather than all the extant code?" sentiment from Paul Eggert.)
Hmmm, can agree to that. At least if we don't continue writing bad code
for the new one. :)
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: termios constants should be unsigned
From: Paul Eggert @ 2024-06-12 18:27 UTC (permalink / raw)
To: Alejandro Colomar
Cc: Andrew Morton, Palmer Dabbelt, linux-api, libc-alpha, linux-man
In-Reply-To: <mdidkojqnhvf5b22vh3c4b6ajmq5miuyr3ole26kx2qkmnbfh3@woy2ghe5eyve>
On 2024-06-12 09:28, Alejandro Colomar wrote:
> adding U is a net
> improvement, without downsides (or I can't see them)
Adding U can change the generated code and thus cause behavior change in
programs (admittedly not well-written ones). It could also create new
false positives in well-written programs.
If adding U fixed real bugs then of course we should do it. Here, though....
^ permalink raw reply
* Re: termios constants should be unsigned
From: enh @ 2024-06-12 17:47 UTC (permalink / raw)
To: Alejandro Colomar
Cc: Paul Eggert, Andrew Morton, Palmer Dabbelt, linux-api, libc-alpha,
linux-man
In-Reply-To: <mdidkojqnhvf5b22vh3c4b6ajmq5miuyr3ole26kx2qkmnbfh3@woy2ghe5eyve>
On Wed, Jun 12, 2024 at 12:29 PM Alejandro Colomar <alx@kernel.org> wrote:
>
> Hi Paul,
>
> On Wed, Jun 12, 2024 at 07:55:14AM GMT, Paul Eggert wrote:
> > On 2024-06-12 05:16, Alejandro Colomar wrote:
> > > tcgets.c:53:24:
> > > error: implicit conversion changes signedness: 'int' to 'tcflag_t' (aka
> > > 'unsigned int') [clang-diagnostic-sign-conversion,-warnings-as-errors]
> >
> > This is a bug in Clang not glibc, and if you're worried about it I suggest
> > sending a bug report to the Clang folks about the false positive.
> >
> > Even GCC's -Wsign-conversion, which is at least smart enough to not warn
> > about benign conversions like that, is too often so chatty that it's best
> > avoided.
> >
> > A lot of this stuff is pedanticism that dates back to the bad old days when
> > the C standard allowed ones' complement and signed magnitude representations
> > of signed integers. Although it can be amusing to worry about that
> > possibility (I know I've done it) it's never been a practical worry, and
> > even the motivation of pedanticism is going away now that C23 requires two's
> > complement.
>
> I know; I think I have -Weverything enabled in that run, which is known
> for its pedanticity. I usually disable it when it triggers a warning,
> since they are usually nonsense. But in this case, adding U is a net
> improvement, without downsides (or I can't see them).
well, any change like this is a potential source incompatibility ... i
hacked these changes into AOSP, and it did break one bit of existing
code that was already working around the sign differences --- this
warning was enabled but the code had a cast to make the _other_ side
of the comparison signed (rather than make this side of the comparison
unsigned).
Android's libc [bionic] uses the uapi headers directly, so we would be
affected, but to be clear --- i'm fine with this if the consensus is
to go this way.
(but, yeah, i'm with the "how about we fix the language and compiler
rather than all the extant code?" sentiment from Paul Eggert.)
> So, while the kernel and glibc are just fine with this implicit
> conversion, they would be equally fine and even better without the
> conversion. Not a bug, but rather a slight improvement.
>
> Have a lovely day!
> Alex
>
> --
> <https://www.alejandro-colomar.es/>
^ permalink raw reply
* Re: [PATCH v1 11/14] futex: Implement FUTEX2_NUMA
From: Peter Zijlstra @ 2024-06-12 17:44 UTC (permalink / raw)
To: Christoph Lameter (Ampere)
Cc: tglx, axboe, linux-kernel, mingo, dvhart, dave, andrealmeid,
Andrew Morton, urezki, hch, lstoakes, Arnd Bergmann, linux-api,
linux-mm, linux-arch, malteskarupke
In-Reply-To: <9dc04e4c-2adc-5084-4ea1-b200d82be29f@linux.com>
On Wed, Jun 12, 2024 at 10:23:00AM -0700, Christoph Lameter (Ampere) wrote:
> On Fri, 21 Jul 2023, Peter Zijlstra wrote:
>
> > Extend the futex2 interface to be numa aware.
>
> Sorry to be chiming in that late but it seems that this is useful to
> mitigate NUMA issues also for our platform.
I read this like: I tested it and it works for me. Is that a correct
reading of your statement?
If so, I'll look at bumping this on the priority list and I'll look at
the placement suggestion you had when I respin the patches.
Thanks!
^ permalink raw reply
* Re: [PATCH v1 11/14] futex: Implement FUTEX2_NUMA
From: Christoph Lameter (Ampere) @ 2024-06-12 17:23 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tglx, axboe, linux-kernel, mingo, dvhart, dave, andrealmeid,
Andrew Morton, urezki, hch, lstoakes, Arnd Bergmann, linux-api,
linux-mm, linux-arch, malteskarupke
In-Reply-To: <20230721105744.434742902@infradead.org>
On Fri, 21 Jul 2023, Peter Zijlstra wrote:
> Extend the futex2 interface to be numa aware.
Sorry to be chiming in that late but it seems that this is useful to
mitigate NUMA issues also for our platform.
> When FUTEX2_NUMA is not set, the node is simply an extention of the
> hash, such that traditional futexes are still interleaved over the
> nodes.
Could we follow NUMA policies like with other metadata allocations during
systen call processing? If there is no NUMA task policy then the futex
should be placed on the local NUMA node.
That way the placement of the futex can be controlled by the tasks memory
policy. We could skip the FUTEX2_NUMA option.
> @@ -114,10 +137,29 @@ late_initcall(fail_futex_debugfs);
> */
> struct futex_hash_bucket *futex_hash(union futex_key *key)
> {
> - u32 hash = jhash2((u32 *)key, offsetof(typeof(*key), both.offset) / 4,
> + u32 hash = jhash2((u32 *)key,
> + offsetof(typeof(*key), both.offset) / sizeof(u32),
> key->both.offset);
> + int node = key->both.node;
>
> - return &futex_queues[hash & (futex_hashsize - 1)];
> + if (node == -1) {
> + /*
> + * In case of !FLAGS_NUMA, use some unused hash bits to pick a
> + * node -- this ensures regular futexes are interleaved across
> + * the nodes and avoids having to allocate multiple
> + * hash-tables.
> + *
> + * NOTE: this isn't perfectly uniform, but it is fast and
> + * handles sparse node masks.
> + */
> + node = (hash >> futex_hashshift) % nr_node_ids;
> + if (!node_possible(node)) {
> + node = find_next_bit_wrap(node_possible_map.bits,
> + nr_node_ids, node);
> + }
Use memory allocation policies here instead?
^ permalink raw reply
* Re: [PATCH v1 11/14] futex: Implement FUTEX2_NUMA
From: Christoph Lameter (Ampere) @ 2024-06-12 17:07 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Thomas Gleixner, axboe, linux-kernel, mingo, dvhart, dave,
andrealmeid, Andrew Morton, urezki, hch, lstoakes, Arnd Bergmann,
linux-api, linux-mm, linux-arch, malteskarupke
In-Reply-To: <20230731180320.GR29590@hirez.programming.kicks-ass.net>
On Mon, 31 Jul 2023, Peter Zijlstra wrote:
>> Is nr_node_ids guaranteed to be stable after init? It's marked
>> __read_mostly, but not __ro_after_init.
>
> AFAICT it is only ever written to in setup_nr_node_ids() and that is all
> __init code. So I'm thinking this could/should indeed be
> __ro_after_init. Esp. so since it is an exported variable.
>
> Mike?
Its stable and lots of other components depend on it like f.e. the size of
cpumasks.
^ permalink raw reply
* Re: termios constants should be unsigned
From: Alejandro Colomar @ 2024-06-12 16:28 UTC (permalink / raw)
To: Paul Eggert
Cc: Andrew Morton, Palmer Dabbelt, linux-api, libc-alpha, linux-man
In-Reply-To: <87af5e8f-0dcb-44a0-94de-757cad7d5ded@cs.ucla.edu>
[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]
Hi Paul,
On Wed, Jun 12, 2024 at 07:55:14AM GMT, Paul Eggert wrote:
> On 2024-06-12 05:16, Alejandro Colomar wrote:
> > tcgets.c:53:24:
> > error: implicit conversion changes signedness: 'int' to 'tcflag_t' (aka
> > 'unsigned int') [clang-diagnostic-sign-conversion,-warnings-as-errors]
>
> This is a bug in Clang not glibc, and if you're worried about it I suggest
> sending a bug report to the Clang folks about the false positive.
>
> Even GCC's -Wsign-conversion, which is at least smart enough to not warn
> about benign conversions like that, is too often so chatty that it's best
> avoided.
>
> A lot of this stuff is pedanticism that dates back to the bad old days when
> the C standard allowed ones' complement and signed magnitude representations
> of signed integers. Although it can be amusing to worry about that
> possibility (I know I've done it) it's never been a practical worry, and
> even the motivation of pedanticism is going away now that C23 requires two's
> complement.
I know; I think I have -Weverything enabled in that run, which is known
for its pedanticity. I usually disable it when it triggers a warning,
since they are usually nonsense. But in this case, adding U is a net
improvement, without downsides (or I can't see them).
So, while the kernel and glibc are just fine with this implicit
conversion, they would be equally fine and even better without the
conversion. Not a bug, but rather a slight improvement.
Have a lovely day!
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] uapi/asm/termbits: Use the U integer suffix for bit fields
From: Alejandro Colomar @ 2024-06-12 15:07 UTC (permalink / raw)
To: Greg KH; +Cc: linux-api, linux-man, Andrew Morton, Palmer Dabbelt, libc-alpha
In-Reply-To: <2024061223-saddlebag-gallantly-c35f@gregkh>
[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]
Hi Greg,
On Wed, Jun 12, 2024 at 04:21:25PM GMT, Greg KH wrote:
> On Wed, Jun 12, 2024 at 04:00:18PM +0200, Alejandro Colomar wrote:
> > I expect that these specific values and the operations done on them
> > probably don't trigger UB, since the shifts are done by a controlled
> > amount, and there are justa few operations done on them.
>
> These, for the most part, are NOT used as shifts.
Quoting the EXAMPLES section in the manual page:
tio.c_cflag &= ~(CBAUD << IBSHIFT);
(And yeah, that shift is presumably controlled, so that it doesn't
overflow, which is why I mean these are presumably just fine.)
> > TL;DR: The kernel isn't broken, but improving this would allow users to
> > enable stricter warnings, which is a good thing.
>
> Enable it where?
I meant in user space programs that use termbits stuff. (That this may
also allow the kernel to eventually have stricter warnings, I don't
know. It might help. But mostly meant it for user space.)
So, if I have a user-space program (or more likely a library) which
wraps these ioctls, I'd prefer to be able to enable the warnings I
reported, to preclude any mistakes in my code. That would need the
constants to be unsigned, to avoid false negatives.
Cheers,
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: termios constants should be unsigned
From: Paul Eggert @ 2024-06-12 14:55 UTC (permalink / raw)
To: Alejandro Colomar, Andrew Morton, Palmer Dabbelt, linux-api,
libc-alpha, linux-man
In-Reply-To: <a7kfppfptkzvqys6cblwjudlpoghsycjglw57hxe2ywvruzkbd@e6nqpnxgwfnq>
On 2024-06-12 05:16, Alejandro Colomar wrote:
> tcgets.c:53:24:
> error: implicit conversion changes signedness: 'int' to 'tcflag_t' (aka
> 'unsigned int') [clang-diagnostic-sign-conversion,-warnings-as-errors]
This is a bug in Clang not glibc, and if you're worried about it I
suggest sending a bug report to the Clang folks about the false positive.
Even GCC's -Wsign-conversion, which is at least smart enough to not warn
about benign conversions like that, is too often so chatty that it's
best avoided.
A lot of this stuff is pedanticism that dates back to the bad old days
when the C standard allowed ones' complement and signed magnitude
representations of signed integers. Although it can be amusing to worry
about that possibility (I know I've done it) it's never been a practical
worry, and even the motivation of pedanticism is going away now that C23
requires two's complement.
^ permalink raw reply
* Re: [PATCH] uapi/asm/termbits: Use the U integer suffix for bit fields
From: Greg KH @ 2024-06-12 14:21 UTC (permalink / raw)
To: Alejandro Colomar
Cc: linux-api, linux-man, Andrew Morton, Palmer Dabbelt, libc-alpha
In-Reply-To: <tkfp53faunf5jlszu5k6jwwxgsl72faffiux7c3pykmnowapy6@wap3bvtvoebi>
On Wed, Jun 12, 2024 at 04:00:18PM +0200, Alejandro Colomar wrote:
> Hi Greg,
>
> On Wed, Jun 12, 2024 at 03:35:20PM GMT, Greg KH wrote:
> > On Wed, Jun 12, 2024 at 03:16:58PM +0200, Alejandro Colomar wrote:
> > > Constants that are to be used in bitwise operations should be unsigned,
> > > or a user could easily trigger Undefined Behavior.
> >
> > Wait, do we really have such broken compilers out there?
>
> I meant this as a generic statement that signed integers on bitwise ops
> are bad, not as a specific statement that these values would trigger UB.
>
> I expect that these specific values and the operations done on them
> probably don't trigger UB, since the shifts are done by a controlled
> amount, and there are justa few operations done on them.
These, for the most part, are NOT used as shifts.
> For example, a left shift where a set bit overflows the type (e.g.,
> 1<<32), causes UB.
Sure, but that's not in play here.
> The reason why it's better to avoid this at all even if we know these
> values work fine, is that programs using <asm/termbits.h> would need to
> disable those compiler warnings, which could silence warnings on other
> code which might be broken.
But again, you aren't using these as bit shifts, they are bit masks, or
values, only.
> TL;DR: The kernel isn't broken, but improving this would allow users to
> enable stricter warnings, which is a good thing.
Enable it where?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] uapi/asm/termbits: Use the U integer suffix for bit fields
From: Alejandro Colomar @ 2024-06-12 14:00 UTC (permalink / raw)
To: Greg KH; +Cc: linux-api, linux-man, Andrew Morton, Palmer Dabbelt, libc-alpha
In-Reply-To: <2024061214-absolute-deranged-14bf@gregkh>
[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]
Hi Greg,
On Wed, Jun 12, 2024 at 03:35:20PM GMT, Greg KH wrote:
> On Wed, Jun 12, 2024 at 03:16:58PM +0200, Alejandro Colomar wrote:
> > Constants that are to be used in bitwise operations should be unsigned,
> > or a user could easily trigger Undefined Behavior.
>
> Wait, do we really have such broken compilers out there?
I meant this as a generic statement that signed integers on bitwise ops
are bad, not as a specific statement that these values would trigger UB.
I expect that these specific values and the operations done on them
probably don't trigger UB, since the shifts are done by a controlled
amount, and there are justa few operations done on them.
For example, a left shift where a set bit overflows the type (e.g.,
1<<32), causes UB.
The reason why it's better to avoid this at all even if we know these
values work fine, is that programs using <asm/termbits.h> would need to
disable those compiler warnings, which could silence warnings on other
code which might be broken.
TL;DR: The kernel isn't broken, but improving this would allow users to
enable stricter warnings, which is a good thing.
> With this change, can the glibc versions then be dropped to just rely on
> these instead?
I don't know. glibc is CCd, so they can answer that.
Have a lovely day!
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] uapi/asm/termbits: Use the U integer suffix for bit fields
From: Greg KH @ 2024-06-12 13:35 UTC (permalink / raw)
To: Alejandro Colomar
Cc: linux-api, linux-man, Andrew Morton, Palmer Dabbelt, libc-alpha
In-Reply-To: <20240612131633.449937-2-alx@kernel.org>
On Wed, Jun 12, 2024 at 03:16:58PM +0200, Alejandro Colomar wrote:
> Constants that are to be used in bitwise operations should be unsigned,
> or a user could easily trigger Undefined Behavior.
Wait, do we really have such broken compilers out there? If so, how has
no one hit this before?
> Also, the types where these constants are to be assigned are unsigned,
> so this makes it more consistent.
Fair enough.
> alx@debian:/usr/include$ grepc -tt termios asm-generic/
> asm-generic/termbits.h:struct termios {
> tcflag_t c_iflag; /* input mode flags */
> tcflag_t c_oflag; /* output mode flags */
> tcflag_t c_cflag; /* control mode flags */
> tcflag_t c_lflag; /* local mode flags */
> cc_t c_line; /* line discipline */
> cc_t c_cc[NCCS]; /* control characters */
> };
> alx@debian:/usr/include$ grepc -tt tcflag_t asm-generic/
> asm-generic/termbits.h:typedef unsigned int tcflag_t;
> alx@debian:/usr/include$ grepc -tt cc_t asm-generic/
> asm-generic/termbits-common.h:typedef unsigned char cc_t;
> alx@debian:/usr/include$ grepc -tt speed_t asm-generic/
> asm-generic/termbits-common.h:typedef unsigned int speed_t;
>
> Link: <https://lore.kernel.org/linux-api/2024061222-scuttle-expanse-6438@gregkh/T/>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Palmer Dabbelt <palmer@rivosinc.com>
> Cc: <linux-api@vger.kernel.org>
> Cc: <libc-alpha@sourceware.org>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> ---
>
> Hi Greg,
>
> On Wed, Jun 12, 2024 at 02:22:37PM GMT, Greg KH wrote:
> > Have a proposed patch that you feel would resolve this?
> >
> > thanks,
> >
> > greg k-h
>
> Here it is. :)
>
> For reviewing it, I suggest using '--word-diff-regex=.'.
>
> I compiled the kernel, and it seems ok; didn't test more than that.
With this change, can the glibc versions then be dropped to just rely on
these instead?
thanks,
greg k-h
^ permalink raw reply
* [PATCH] uapi/asm/termbits: Use the U integer suffix for bit fields
From: Alejandro Colomar @ 2024-06-12 13:16 UTC (permalink / raw)
To: linux-api
Cc: Alejandro Colomar, linux-man, Greg KH, Andrew Morton,
Palmer Dabbelt, libc-alpha
In-Reply-To: <2024061222-scuttle-expanse-6438@gregkh>
[-- Attachment #1: Type: text/plain, Size: 34853 bytes --]
Constants that are to be used in bitwise operations should be unsigned,
or a user could easily trigger Undefined Behavior.
Also, the types where these constants are to be assigned are unsigned,
so this makes it more consistent.
alx@debian:/usr/include$ grepc -tt termios asm-generic/
asm-generic/termbits.h:struct termios {
tcflag_t c_iflag; /* input mode flags */
tcflag_t c_oflag; /* output mode flags */
tcflag_t c_cflag; /* control mode flags */
tcflag_t c_lflag; /* local mode flags */
cc_t c_line; /* line discipline */
cc_t c_cc[NCCS]; /* control characters */
};
alx@debian:/usr/include$ grepc -tt tcflag_t asm-generic/
asm-generic/termbits.h:typedef unsigned int tcflag_t;
alx@debian:/usr/include$ grepc -tt cc_t asm-generic/
asm-generic/termbits-common.h:typedef unsigned char cc_t;
alx@debian:/usr/include$ grepc -tt speed_t asm-generic/
asm-generic/termbits-common.h:typedef unsigned int speed_t;
Link: <https://lore.kernel.org/linux-api/2024061222-scuttle-expanse-6438@gregkh/T/>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Palmer Dabbelt <palmer@rivosinc.com>
Cc: <linux-api@vger.kernel.org>
Cc: <libc-alpha@sourceware.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
Hi Greg,
On Wed, Jun 12, 2024 at 02:22:37PM GMT, Greg KH wrote:
> Have a proposed patch that you feel would resolve this?
>
> thanks,
>
> greg k-h
Here it is. :)
For reviewing it, I suggest using '--word-diff-regex=.'.
I compiled the kernel, and it seems ok; didn't test more than that.
Have a lovely day!
Alex
Range-diff:
-: ------------ > 1: 588eb01ac153 uapi/asm/termbits: Use the U integer suffix for bit fields
arch/alpha/include/uapi/asm/termbits.h | 154 +++++++++---------
arch/mips/include/uapi/asm/termbits.h | 154 +++++++++---------
arch/parisc/include/uapi/asm/termbits.h | 152 +++++++++---------
arch/powerpc/include/uapi/asm/termbits.h | 156 +++++++++---------
arch/sparc/include/uapi/asm/termbits.h | 192 +++++++++++------------
include/uapi/asm-generic/termbits.h | 152 +++++++++---------
6 files changed, 480 insertions(+), 480 deletions(-)
diff --git a/arch/alpha/include/uapi/asm/termbits.h b/arch/alpha/include/uapi/asm/termbits.h
index f1290b22072b..965a8c93523d 100644
--- a/arch/alpha/include/uapi/asm/termbits.h
+++ b/arch/alpha/include/uapi/asm/termbits.h
@@ -70,39 +70,39 @@ struct ktermios {
#define VTIME 17
/* c_iflag bits */
-#define IXON 0x0200
-#define IXOFF 0x0400
-#define IUCLC 0x1000
-#define IMAXBEL 0x2000
-#define IUTF8 0x4000
+#define IXON 0x0200U
+#define IXOFF 0x0400U
+#define IUCLC 0x1000U
+#define IMAXBEL 0x2000U
+#define IUTF8 0x4000U
/* c_oflag bits */
-#define ONLCR 0x00002
-#define OLCUC 0x00004
-#define NLDLY 0x00300
-#define NL0 0x00000
-#define NL1 0x00100
-#define NL2 0x00200
-#define NL3 0x00300
-#define TABDLY 0x00c00
-#define TAB0 0x00000
-#define TAB1 0x00400
-#define TAB2 0x00800
-#define TAB3 0x00c00
-#define CRDLY 0x03000
-#define CR0 0x00000
-#define CR1 0x01000
-#define CR2 0x02000
-#define CR3 0x03000
-#define FFDLY 0x04000
-#define FF0 0x00000
-#define FF1 0x04000
-#define BSDLY 0x08000
-#define BS0 0x00000
-#define BS1 0x08000
-#define VTDLY 0x10000
-#define VT0 0x00000
-#define VT1 0x10000
+#define ONLCR 0x00002U
+#define OLCUC 0x00004U
+#define NLDLY 0x00300U
+#define NL0 0x00000U
+#define NL1 0x00100U
+#define NL2 0x00200U
+#define NL3 0x00300U
+#define TABDLY 0x00c00U
+#define TAB0 0x00000U
+#define TAB1 0x00400U
+#define TAB2 0x00800U
+#define TAB3 0x00c00U
+#define CRDLY 0x03000U
+#define CR0 0x00000U
+#define CR1 0x01000U
+#define CR2 0x02000U
+#define CR3 0x03000U
+#define FFDLY 0x04000U
+#define FF0 0x00000U
+#define FF1 0x04000U
+#define BSDLY 0x08000U
+#define BS0 0x00000U
+#define BS1 0x08000U
+#define VTDLY 0x10000U
+#define VT0 0x00000U
+#define VT1 0x10000U
/*
* Should be equivalent to TAB3, see description of TAB3 in
* POSIX.1-2008, Ch. 11.2.3 "Output Modes"
@@ -110,54 +110,54 @@ struct ktermios {
#define XTABS TAB3
/* c_cflag bit meaning */
-#define CBAUD 0x0000001f
-#define CBAUDEX 0x00000000
-#define BOTHER 0x0000001f
-#define B57600 0x00000010
-#define B115200 0x00000011
-#define B230400 0x00000012
-#define B460800 0x00000013
-#define B500000 0x00000014
-#define B576000 0x00000015
-#define B921600 0x00000016
-#define B1000000 0x00000017
-#define B1152000 0x00000018
-#define B1500000 0x00000019
-#define B2000000 0x0000001a
-#define B2500000 0x0000001b
-#define B3000000 0x0000001c
-#define B3500000 0x0000001d
-#define B4000000 0x0000001e
-#define CSIZE 0x00000300
-#define CS5 0x00000000
-#define CS6 0x00000100
-#define CS7 0x00000200
-#define CS8 0x00000300
-#define CSTOPB 0x00000400
-#define CREAD 0x00000800
-#define PARENB 0x00001000
-#define PARODD 0x00002000
-#define HUPCL 0x00004000
-#define CLOCAL 0x00008000
-#define CIBAUD 0x001f0000
+#define CBAUD 0x0000001fU
+#define CBAUDEX 0x00000000U
+#define BOTHER 0x0000001fU
+#define B57600 0x00000010U
+#define B115200 0x00000011U
+#define B230400 0x00000012U
+#define B460800 0x00000013U
+#define B500000 0x00000014U
+#define B576000 0x00000015U
+#define B921600 0x00000016U
+#define B1000000 0x00000017U
+#define B1152000 0x00000018U
+#define B1500000 0x00000019U
+#define B2000000 0x0000001aU
+#define B2500000 0x0000001bU
+#define B3000000 0x0000001cU
+#define B3500000 0x0000001dU
+#define B4000000 0x0000001eU
+#define CSIZE 0x00000300U
+#define CS5 0x00000000U
+#define CS6 0x00000100U
+#define CS7 0x00000200U
+#define CS8 0x00000300U
+#define CSTOPB 0x00000400U
+#define CREAD 0x00000800U
+#define PARENB 0x00001000U
+#define PARODD 0x00002000U
+#define HUPCL 0x00004000U
+#define CLOCAL 0x00008000U
+#define CIBAUD 0x001f0000U
/* c_lflag bits */
-#define ISIG 0x00000080
-#define ICANON 0x00000100
-#define XCASE 0x00004000
-#define ECHO 0x00000008
-#define ECHOE 0x00000002
-#define ECHOK 0x00000004
-#define ECHONL 0x00000010
-#define NOFLSH 0x80000000
-#define TOSTOP 0x00400000
-#define ECHOCTL 0x00000040
-#define ECHOPRT 0x00000020
-#define ECHOKE 0x00000001
-#define FLUSHO 0x00800000
-#define PENDIN 0x20000000
-#define IEXTEN 0x00000400
-#define EXTPROC 0x10000000
+#define ISIG 0x00000080U
+#define ICANON 0x00000100U
+#define XCASE 0x00004000U
+#define ECHO 0x00000008U
+#define ECHOE 0x00000002U
+#define ECHOK 0x00000004U
+#define ECHONL 0x00000010U
+#define NOFLSH 0x80000000U
+#define TOSTOP 0x00400000U
+#define ECHOCTL 0x00000040U
+#define ECHOPRT 0x00000020U
+#define ECHOKE 0x00000001U
+#define FLUSHO 0x00800000U
+#define PENDIN 0x20000000U
+#define IEXTEN 0x00000400U
+#define EXTPROC 0x10000000U
/* Values for the OPTIONAL_ACTIONS argument to `tcsetattr'. */
#define TCSANOW 0
diff --git a/arch/mips/include/uapi/asm/termbits.h b/arch/mips/include/uapi/asm/termbits.h
index 1eb60903d6f0..35eb388a176e 100644
--- a/arch/mips/include/uapi/asm/termbits.h
+++ b/arch/mips/include/uapi/asm/termbits.h
@@ -78,96 +78,96 @@ struct ktermios {
#define VEOL 17 /* End-of-line character [ICANON] */
/* c_iflag bits */
-#define IUCLC 0x0200 /* Map upper case to lower case on input */
-#define IXON 0x0400 /* Enable start/stop output control */
-#define IXOFF 0x1000 /* Enable start/stop input control */
-#define IMAXBEL 0x2000 /* Ring bell when input queue is full */
-#define IUTF8 0x4000 /* Input is UTF-8 */
+#define IUCLC 0x0200U /* Map upper case to lower case on input */
+#define IXON 0x0400U /* Enable start/stop output control */
+#define IXOFF 0x1000U /* Enable start/stop input control */
+#define IMAXBEL 0x2000U /* Ring bell when input queue is full */
+#define IUTF8 0x4000U /* Input is UTF-8 */
/* c_oflag bits */
-#define OLCUC 0x00002 /* Map lower case to upper case on output */
-#define ONLCR 0x00004 /* Map NL to CR-NL on output */
-#define NLDLY 0x00100
-#define NL0 0x00000
-#define NL1 0x00100
-#define CRDLY 0x00600
-#define CR0 0x00000
-#define CR1 0x00200
-#define CR2 0x00400
-#define CR3 0x00600
-#define TABDLY 0x01800
-#define TAB0 0x00000
-#define TAB1 0x00800
-#define TAB2 0x01000
-#define TAB3 0x01800
-#define XTABS 0x01800
-#define BSDLY 0x02000
-#define BS0 0x00000
-#define BS1 0x02000
-#define VTDLY 0x04000
-#define VT0 0x00000
-#define VT1 0x04000
-#define FFDLY 0x08000
-#define FF0 0x00000
-#define FF1 0x08000
+#define OLCUC 0x00002U /* Map lower case to upper case on output */
+#define ONLCR 0x00004U /* Map NL to CR-NL on output */
+#define NLDLY 0x00100U
+#define NL0 0x00000U
+#define NL1 0x00100U
+#define CRDLY 0x00600U
+#define CR0 0x00000U
+#define CR1 0x00200U
+#define CR2 0x00400U
+#define CR3 0x00600U
+#define TABDLY 0x01800U
+#define TAB0 0x00000U
+#define TAB1 0x00800U
+#define TAB2 0x01000U
+#define TAB3 0x01800U
+#define XTABS 0x01800U
+#define BSDLY 0x02000U
+#define BS0 0x00000U
+#define BS1 0x02000U
+#define VTDLY 0x04000U
+#define VT0 0x00000U
+#define VT1 0x04000U
+#define FFDLY 0x08000U
+#define FF0 0x00000U
+#define FF1 0x08000U
/*
#define PAGEOUT ???
#define WRAP ???
*/
/* c_cflag bit meaning */
-#define CBAUD 0x0000100f
-#define CSIZE 0x00000030 /* Number of bits per byte (mask) */
-#define CS5 0x00000000 /* 5 bits per byte */
-#define CS6 0x00000010 /* 6 bits per byte */
-#define CS7 0x00000020 /* 7 bits per byte */
-#define CS8 0x00000030 /* 8 bits per byte */
-#define CSTOPB 0x00000040 /* Two stop bits instead of one */
-#define CREAD 0x00000080 /* Enable receiver */
-#define PARENB 0x00000100 /* Parity enable */
-#define PARODD 0x00000200 /* Odd parity instead of even */
-#define HUPCL 0x00000400 /* Hang up on last close */
-#define CLOCAL 0x00000800 /* Ignore modem status lines */
-#define CBAUDEX 0x00001000
-#define BOTHER 0x00001000
-#define B57600 0x00001001
-#define B115200 0x00001002
-#define B230400 0x00001003
-#define B460800 0x00001004
-#define B500000 0x00001005
-#define B576000 0x00001006
-#define B921600 0x00001007
-#define B1000000 0x00001008
-#define B1152000 0x00001009
-#define B1500000 0x0000100a
-#define B2000000 0x0000100b
-#define B2500000 0x0000100c
-#define B3000000 0x0000100d
-#define B3500000 0x0000100e
-#define B4000000 0x0000100f
-#define CIBAUD 0x100f0000 /* input baud rate */
+#define CBAUD 0x0000100fU
+#define CSIZE 0x00000030U /* Number of bits per byte (mask) */
+#define CS5 0x00000000U /* 5 bits per byte */
+#define CS6 0x00000010U /* 6 bits per byte */
+#define CS7 0x00000020U /* 7 bits per byte */
+#define CS8 0x00000030U /* 8 bits per byte */
+#define CSTOPB 0x00000040U /* Two stop bits instead of one */
+#define CREAD 0x00000080U /* Enable receiver */
+#define PARENB 0x00000100U /* Parity enable */
+#define PARODD 0x00000200U /* Odd parity instead of even */
+#define HUPCL 0x00000400U /* Hang up on last close */
+#define CLOCAL 0x00000800U /* Ignore modem status lines */
+#define CBAUDEX 0x00001000U
+#define BOTHER 0x00001000U
+#define B57600 0x00001001U
+#define B115200 0x00001002U
+#define B230400 0x00001003U
+#define B460800 0x00001004U
+#define B500000 0x00001005U
+#define B576000 0x00001006U
+#define B921600 0x00001007U
+#define B1000000 0x00001008U
+#define B1152000 0x00001009U
+#define B1500000 0x0000100aU
+#define B2000000 0x0000100bU
+#define B2500000 0x0000100cU
+#define B3000000 0x0000100dU
+#define B3500000 0x0000100eU
+#define B4000000 0x0000100fU
+#define CIBAUD 0x100f0000U /* input baud rate */
/* c_lflag bits */
-#define ISIG 0x00001 /* Enable signals */
-#define ICANON 0x00002 /* Do erase and kill processing */
-#define XCASE 0x00004
-#define ECHO 0x00008 /* Enable echo */
-#define ECHOE 0x00010 /* Visual erase for ERASE */
-#define ECHOK 0x00020 /* Echo NL after KILL */
-#define ECHONL 0x00040 /* Echo NL even if ECHO is off */
-#define NOFLSH 0x00080 /* Disable flush after interrupt */
-#define IEXTEN 0x00100 /* Enable DISCARD and LNEXT */
-#define ECHOCTL 0x00200 /* Echo control characters as ^X */
-#define ECHOPRT 0x00400 /* Hardcopy visual erase */
-#define ECHOKE 0x00800 /* Visual erase for KILL */
-#define FLUSHO 0x02000
-#define PENDIN 0x04000 /* Retype pending input (state) */
-#define TOSTOP 0x08000 /* Send SIGTTOU for background output */
+#define ISIG 0x00001U /* Enable signals */
+#define ICANON 0x00002U /* Do erase and kill processing */
+#define XCASE 0x00004U
+#define ECHO 0x00008U /* Enable echo */
+#define ECHOE 0x00010U /* Visual erase for ERASE */
+#define ECHOK 0x00020U /* Echo NL after KILL */
+#define ECHONL 0x00040U /* Echo NL even if ECHO is off */
+#define NOFLSH 0x00080U /* Disable flush after interrupt */
+#define IEXTEN 0x00100U /* Enable DISCARD and LNEXT */
+#define ECHOCTL 0x00200U /* Echo control characters as ^X */
+#define ECHOPRT 0x00400U /* Hardcopy visual erase */
+#define ECHOKE 0x00800U /* Visual erase for KILL */
+#define FLUSHO 0x02000U
+#define PENDIN 0x04000U /* Retype pending input (state) */
+#define TOSTOP 0x08000U /* Send SIGTTOU for background output */
#define ITOSTOP TOSTOP
-#define EXTPROC 0x10000 /* External processing on pty */
+#define EXTPROC 0x10000U /* External processing on pty */
/* ioctl (fd, TIOCSERGETLSR, &result) where result may be as below */
-#define TIOCSER_TEMT 0x01 /* Transmitter physically empty */
+#define TIOCSER_TEMT 0x01U /* Transmitter physically empty */
/* tcsetattr uses these */
#define TCSANOW TCSETS /* Change immediately */
diff --git a/arch/parisc/include/uapi/asm/termbits.h b/arch/parisc/include/uapi/asm/termbits.h
index 3a8938d26fb4..d9090878c129 100644
--- a/arch/parisc/include/uapi/asm/termbits.h
+++ b/arch/parisc/include/uapi/asm/termbits.h
@@ -58,88 +58,88 @@ struct ktermios {
#define VEOL2 16
/* c_iflag bits */
-#define IUCLC 0x0200
-#define IXON 0x0400
-#define IXOFF 0x1000
-#define IMAXBEL 0x4000
-#define IUTF8 0x8000
+#define IUCLC 0x0200U
+#define IXON 0x0400U
+#define IXOFF 0x1000U
+#define IMAXBEL 0x4000U
+#define IUTF8 0x8000U
/* c_oflag bits */
-#define OLCUC 0x00002
-#define ONLCR 0x00004
-#define NLDLY 0x00100
-#define NL0 0x00000
-#define NL1 0x00100
-#define CRDLY 0x00600
-#define CR0 0x00000
-#define CR1 0x00200
-#define CR2 0x00400
-#define CR3 0x00600
-#define TABDLY 0x01800
-#define TAB0 0x00000
-#define TAB1 0x00800
-#define TAB2 0x01000
-#define TAB3 0x01800
-#define XTABS 0x01800
-#define BSDLY 0x02000
-#define BS0 0x00000
-#define BS1 0x02000
-#define VTDLY 0x04000
-#define VT0 0x00000
-#define VT1 0x04000
-#define FFDLY 0x08000
-#define FF0 0x00000
-#define FF1 0x08000
+#define OLCUC 0x00002U
+#define ONLCR 0x00004U
+#define NLDLY 0x00100U
+#define NL0 0x00000U
+#define NL1 0x00100U
+#define CRDLY 0x00600U
+#define CR0 0x00000U
+#define CR1 0x00200U
+#define CR2 0x00400U
+#define CR3 0x00600U
+#define TABDLY 0x01800U
+#define TAB0 0x00000U
+#define TAB1 0x00800U
+#define TAB2 0x01000U
+#define TAB3 0x01800U
+#define XTABS 0x01800U
+#define BSDLY 0x02000U
+#define BS0 0x00000U
+#define BS1 0x02000U
+#define VTDLY 0x04000U
+#define VT0 0x00000U
+#define VT1 0x04000U
+#define FFDLY 0x08000U
+#define FF0 0x00000U
+#define FF1 0x08000U
/* c_cflag bit meaning */
-#define CBAUD 0x0000100f
-#define CSIZE 0x00000030
-#define CS5 0x00000000
-#define CS6 0x00000010
-#define CS7 0x00000020
-#define CS8 0x00000030
-#define CSTOPB 0x00000040
-#define CREAD 0x00000080
-#define PARENB 0x00000100
-#define PARODD 0x00000200
-#define HUPCL 0x00000400
-#define CLOCAL 0x00000800
-#define CBAUDEX 0x00001000
-#define BOTHER 0x00001000
-#define B57600 0x00001001
-#define B115200 0x00001002
-#define B230400 0x00001003
-#define B460800 0x00001004
-#define B500000 0x00001005
-#define B576000 0x00001006
-#define B921600 0x00001007
-#define B1000000 0x00001008
-#define B1152000 0x00001009
-#define B1500000 0x0000100a
-#define B2000000 0x0000100b
-#define B2500000 0x0000100c
-#define B3000000 0x0000100d
-#define B3500000 0x0000100e
-#define B4000000 0x0000100f
-#define CIBAUD 0x100f0000 /* input baud rate */
+#define CBAUD 0x0000100fU
+#define CSIZE 0x00000030U
+#define CS5 0x00000000U
+#define CS6 0x00000010U
+#define CS7 0x00000020U
+#define CS8 0x00000030U
+#define CSTOPB 0x00000040U
+#define CREAD 0x00000080U
+#define PARENB 0x00000100U
+#define PARODD 0x00000200U
+#define HUPCL 0x00000400U
+#define CLOCAL 0x00000800U
+#define CBAUDEX 0x00001000U
+#define BOTHER 0x00001000U
+#define B57600 0x00001001U
+#define B115200 0x00001002U
+#define B230400 0x00001003U
+#define B460800 0x00001004U
+#define B500000 0x00001005U
+#define B576000 0x00001006U
+#define B921600 0x00001007U
+#define B1000000 0x00001008U
+#define B1152000 0x00001009U
+#define B1500000 0x0000100aU
+#define B2000000 0x0000100bU
+#define B2500000 0x0000100cU
+#define B3000000 0x0000100dU
+#define B3500000 0x0000100eU
+#define B4000000 0x0000100fU
+#define CIBAUD 0x100f0000U /* input baud rate */
/* c_lflag bits */
-#define ISIG 0x00001
-#define ICANON 0x00002
-#define XCASE 0x00004
-#define ECHO 0x00008
-#define ECHOE 0x00010
-#define ECHOK 0x00020
-#define ECHONL 0x00040
-#define NOFLSH 0x00080
-#define TOSTOP 0x00100
-#define ECHOCTL 0x00200
-#define ECHOPRT 0x00400
-#define ECHOKE 0x00800
-#define FLUSHO 0x01000
-#define PENDIN 0x04000
-#define IEXTEN 0x08000
-#define EXTPROC 0x10000
+#define ISIG 0x00001U
+#define ICANON 0x00002U
+#define XCASE 0x00004U
+#define ECHO 0x00008U
+#define ECHOE 0x00010U
+#define ECHOK 0x00020U
+#define ECHONL 0x00040U
+#define NOFLSH 0x00080U
+#define TOSTOP 0x00100U
+#define ECHOCTL 0x00200U
+#define ECHOPRT 0x00400U
+#define ECHOKE 0x00800U
+#define FLUSHO 0x01000U
+#define PENDIN 0x04000U
+#define IEXTEN 0x08000U
+#define EXTPROC 0x10000U
/* tcsetattr uses these */
#define TCSANOW 0
diff --git a/arch/powerpc/include/uapi/asm/termbits.h b/arch/powerpc/include/uapi/asm/termbits.h
index 21dc86dcb2f1..ae3c2ed3dae8 100644
--- a/arch/powerpc/include/uapi/asm/termbits.h
+++ b/arch/powerpc/include/uapi/asm/termbits.h
@@ -64,90 +64,90 @@ struct ktermios {
#define VDISCARD 16
/* c_iflag bits */
-#define IXON 0x0200
-#define IXOFF 0x0400
-#define IUCLC 0x1000
-#define IMAXBEL 0x2000
-#define IUTF8 0x4000
+#define IXON 0x0200U
+#define IXOFF 0x0400U
+#define IUCLC 0x1000U
+#define IMAXBEL 0x2000U
+#define IUTF8 0x4000U
/* c_oflag bits */
-#define ONLCR 0x00002
-#define OLCUC 0x00004
-#define NLDLY 0x00300
-#define NL0 0x00000
-#define NL1 0x00100
-#define NL2 0x00200
-#define NL3 0x00300
-#define TABDLY 0x00c00
-#define TAB0 0x00000
-#define TAB1 0x00400
-#define TAB2 0x00800
-#define TAB3 0x00c00
-#define XTABS 0x00c00 /* required by POSIX to == TAB3 */
-#define CRDLY 0x03000
-#define CR0 0x00000
-#define CR1 0x01000
-#define CR2 0x02000
-#define CR3 0x03000
-#define FFDLY 0x04000
-#define FF0 0x00000
-#define FF1 0x04000
-#define BSDLY 0x08000
-#define BS0 0x00000
-#define BS1 0x08000
-#define VTDLY 0x10000
-#define VT0 0x00000
-#define VT1 0x10000
+#define ONLCR 0x00002U
+#define OLCUC 0x00004U
+#define NLDLY 0x00300U
+#define NL0 0x00000U
+#define NL1 0x00100U
+#define NL2 0x00200U
+#define NL3 0x00300U
+#define TABDLY 0x00c00U
+#define TAB0 0x00000U
+#define TAB1 0x00400U
+#define TAB2 0x00800U
+#define TAB3 0x00c00U
+#define XTABS 0x00c00U /* required by POSIX to == TAB3 */
+#define CRDLY 0x03000U
+#define CR0 0x00000U
+#define CR1 0x01000U
+#define CR2 0x02000U
+#define CR3 0x03000U
+#define FFDLY 0x04000U
+#define FF0 0x00000U
+#define FF1 0x04000U
+#define BSDLY 0x08000U
+#define BS0 0x00000U
+#define BS1 0x08000U
+#define VTDLY 0x10000U
+#define VT0 0x00000U
+#define VT1 0x10000U
/* c_cflag bit meaning */
-#define CBAUD 0x000000ff
-#define CBAUDEX 0x00000000
-#define BOTHER 0x0000001f
-#define B57600 0x00000010
-#define B115200 0x00000011
-#define B230400 0x00000012
-#define B460800 0x00000013
-#define B500000 0x00000014
-#define B576000 0x00000015
-#define B921600 0x00000016
-#define B1000000 0x00000017
-#define B1152000 0x00000018
-#define B1500000 0x00000019
-#define B2000000 0x0000001a
-#define B2500000 0x0000001b
-#define B3000000 0x0000001c
-#define B3500000 0x0000001d
-#define B4000000 0x0000001e
-#define CSIZE 0x00000300
-#define CS5 0x00000000
-#define CS6 0x00000100
-#define CS7 0x00000200
-#define CS8 0x00000300
-#define CSTOPB 0x00000400
-#define CREAD 0x00000800
-#define PARENB 0x00001000
-#define PARODD 0x00002000
-#define HUPCL 0x00004000
-#define CLOCAL 0x00008000
-#define CIBAUD 0x00ff0000
+#define CBAUD 0x000000ffU
+#define CBAUDEX 0x00000000U
+#define BOTHER 0x0000001fU
+#define B57600 0x00000010U
+#define B115200 0x00000011U
+#define B230400 0x00000012U
+#define B460800 0x00000013U
+#define B500000 0x00000014U
+#define B576000 0x00000015U
+#define B921600 0x00000016U
+#define B1000000 0x00000017U
+#define B1152000 0x00000018U
+#define B1500000 0x00000019U
+#define B2000000 0x0000001aU
+#define B2500000 0x0000001bU
+#define B3000000 0x0000001cU
+#define B3500000 0x0000001dU
+#define B4000000 0x0000001eU
+#define CSIZE 0x00000300U
+#define CS5 0x00000000U
+#define CS6 0x00000100U
+#define CS7 0x00000200U
+#define CS8 0x00000300U
+#define CSTOPB 0x00000400U
+#define CREAD 0x00000800U
+#define PARENB 0x00001000U
+#define PARODD 0x00002000U
+#define HUPCL 0x00004000U
+#define CLOCAL 0x00008000U
+#define CIBAUD 0x00ff0000U
/* c_lflag bits */
-#define ISIG 0x00000080
-#define ICANON 0x00000100
-#define XCASE 0x00004000
-#define ECHO 0x00000008
-#define ECHOE 0x00000002
-#define ECHOK 0x00000004
-#define ECHONL 0x00000010
-#define NOFLSH 0x80000000
-#define TOSTOP 0x00400000
-#define ECHOCTL 0x00000040
-#define ECHOPRT 0x00000020
-#define ECHOKE 0x00000001
-#define FLUSHO 0x00800000
-#define PENDIN 0x20000000
-#define IEXTEN 0x00000400
-#define EXTPROC 0x10000000
+#define ISIG 0x00000080U
+#define ICANON 0x00000100U
+#define XCASE 0x00004000U
+#define ECHO 0x00000008U
+#define ECHOE 0x00000002U
+#define ECHOK 0x00000004U
+#define ECHONL 0x00000010U
+#define NOFLSH 0x80000000U
+#define TOSTOP 0x00400000U
+#define ECHOCTL 0x00000040U
+#define ECHOPRT 0x00000020U
+#define ECHOKE 0x00000001U
+#define FLUSHO 0x00800000U
+#define PENDIN 0x20000000U
+#define IEXTEN 0x00000400U
+#define EXTPROC 0x10000000U
/* Values for the OPTIONAL_ACTIONS argument to `tcsetattr'. */
#define TCSANOW 0
diff --git a/arch/sparc/include/uapi/asm/termbits.h b/arch/sparc/include/uapi/asm/termbits.h
index 0da2b1adc0f5..5cd0b28b16dc 100644
--- a/arch/sparc/include/uapi/asm/termbits.h
+++ b/arch/sparc/include/uapi/asm/termbits.h
@@ -75,121 +75,121 @@ struct ktermios {
#endif
/* c_iflag bits */
-#define IUCLC 0x0200
-#define IXON 0x0400
-#define IXOFF 0x1000
-#define IMAXBEL 0x2000
-#define IUTF8 0x4000
+#define IUCLC 0x0200U
+#define IXON 0x0400U
+#define IXOFF 0x1000U
+#define IMAXBEL 0x2000U
+#define IUTF8 0x4000U
/* c_oflag bits */
-#define OLCUC 0x00002
-#define ONLCR 0x00004
-#define NLDLY 0x00100
-#define NL0 0x00000
-#define NL1 0x00100
-#define CRDLY 0x00600
-#define CR0 0x00000
-#define CR1 0x00200
-#define CR2 0x00400
-#define CR3 0x00600
-#define TABDLY 0x01800
-#define TAB0 0x00000
-#define TAB1 0x00800
-#define TAB2 0x01000
-#define TAB3 0x01800
-#define XTABS 0x01800
-#define BSDLY 0x02000
-#define BS0 0x00000
-#define BS1 0x02000
-#define VTDLY 0x04000
-#define VT0 0x00000
-#define VT1 0x04000
-#define FFDLY 0x08000
-#define FF0 0x00000
-#define FF1 0x08000
-#define PAGEOUT 0x10000 /* SUNOS specific */
-#define WRAP 0x20000 /* SUNOS specific */
+#define OLCUC 0x00002U
+#define ONLCR 0x00004U
+#define NLDLY 0x00100U
+#define NL0 0x00000U
+#define NL1 0x00100U
+#define CRDLY 0x00600U
+#define CR0 0x00000U
+#define CR1 0x00200U
+#define CR2 0x00400U
+#define CR3 0x00600U
+#define TABDLY 0x01800U
+#define TAB0 0x00000U
+#define TAB1 0x00800U
+#define TAB2 0x01000U
+#define TAB3 0x01800U
+#define XTABS 0x01800U
+#define BSDLY 0x02000U
+#define BS0 0x00000U
+#define BS1 0x02000U
+#define VTDLY 0x04000U
+#define VT0 0x00000U
+#define VT1 0x04000U
+#define FFDLY 0x08000U
+#define FF0 0x00000U
+#define FF1 0x08000U
+#define PAGEOUT 0x10000U /* SUNOS specific */
+#define WRAP 0x20000U /* SUNOS specific */
/* c_cflag bit meaning */
-#define CBAUD 0x0000100f
-#define CSIZE 0x00000030
-#define CS5 0x00000000
-#define CS6 0x00000010
-#define CS7 0x00000020
-#define CS8 0x00000030
-#define CSTOPB 0x00000040
-#define CREAD 0x00000080
-#define PARENB 0x00000100
-#define PARODD 0x00000200
-#define HUPCL 0x00000400
-#define CLOCAL 0x00000800
-#define CBAUDEX 0x00001000
+#define CBAUD 0x0000100fU
+#define CSIZE 0x00000030U
+#define CS5 0x00000000U
+#define CS6 0x00000010U
+#define CS7 0x00000020U
+#define CS8 0x00000030U
+#define CSTOPB 0x00000040U
+#define CREAD 0x00000080U
+#define PARENB 0x00000100U
+#define PARODD 0x00000200U
+#define HUPCL 0x00000400U
+#define CLOCAL 0x00000800U
+#define CBAUDEX 0x00001000U
/* We'll never see these speeds with the Zilogs, but for completeness... */
-#define BOTHER 0x00001000
-#define B57600 0x00001001
-#define B115200 0x00001002
-#define B230400 0x00001003
-#define B460800 0x00001004
+#define BOTHER 0x00001000U
+#define B57600 0x00001001U
+#define B115200 0x00001002U
+#define B230400 0x00001003U
+#define B460800 0x00001004U
/* This is what we can do with the Zilogs. */
-#define B76800 0x00001005
+#define B76800 0x00001005U
/* This is what we can do with the SAB82532. */
-#define B153600 0x00001006
-#define B307200 0x00001007
-#define B614400 0x00001008
-#define B921600 0x00001009
+#define B153600 0x00001006U
+#define B307200 0x00001007U
+#define B614400 0x00001008U
+#define B921600 0x00001009U
/* And these are the rest... */
-#define B500000 0x0000100a
-#define B576000 0x0000100b
-#define B1000000 0x0000100c
-#define B1152000 0x0000100d
-#define B1500000 0x0000100e
-#define B2000000 0x0000100f
+#define B500000 0x0000100aU
+#define B576000 0x0000100bU
+#define B1000000 0x0000100cU
+#define B1152000 0x0000100dU
+#define B1500000 0x0000100eU
+#define B2000000 0x0000100fU
/* These have totally bogus values and nobody uses them
so far. Later on we'd have to use say 0x10000x and
adjust CBAUD constant and drivers accordingly.
-#define B2500000 0x00001010
-#define B3000000 0x00001011
-#define B3500000 0x00001012
-#define B4000000 0x00001013 */
-#define CIBAUD 0x100f0000 /* input baud rate (not used) */
+#define B2500000 0x00001010U
+#define B3000000 0x00001011U
+#define B3500000 0x00001012U
+#define B4000000 0x00001013U */
+#define CIBAUD 0x100f0000U /* input baud rate (not used) */
/* c_lflag bits */
-#define ISIG 0x00000001
-#define ICANON 0x00000002
-#define XCASE 0x00000004
-#define ECHO 0x00000008
-#define ECHOE 0x00000010
-#define ECHOK 0x00000020
-#define ECHONL 0x00000040
-#define NOFLSH 0x00000080
-#define TOSTOP 0x00000100
-#define ECHOCTL 0x00000200
-#define ECHOPRT 0x00000400
-#define ECHOKE 0x00000800
-#define DEFECHO 0x00001000 /* SUNOS thing, what is it? */
-#define FLUSHO 0x00002000
-#define PENDIN 0x00004000
-#define IEXTEN 0x00008000
-#define EXTPROC 0x00010000
+#define ISIG 0x00000001U
+#define ICANON 0x00000002U
+#define XCASE 0x00000004U
+#define ECHO 0x00000008U
+#define ECHOE 0x00000010U
+#define ECHOK 0x00000020U
+#define ECHONL 0x00000040U
+#define NOFLSH 0x00000080U
+#define TOSTOP 0x00000100U
+#define ECHOCTL 0x00000200U
+#define ECHOPRT 0x00000400U
+#define ECHOKE 0x00000800U
+#define DEFECHO 0x00001000U /* SUNOS thing, what is it? */
+#define FLUSHO 0x00002000U
+#define PENDIN 0x00004000U
+#define IEXTEN 0x00008000U
+#define EXTPROC 0x00010000U
/* modem lines */
-#define TIOCM_LE 0x001
-#define TIOCM_DTR 0x002
-#define TIOCM_RTS 0x004
-#define TIOCM_ST 0x008
-#define TIOCM_SR 0x010
-#define TIOCM_CTS 0x020
-#define TIOCM_CAR 0x040
-#define TIOCM_RNG 0x080
-#define TIOCM_DSR 0x100
+#define TIOCM_LE 0x001U
+#define TIOCM_DTR 0x002U
+#define TIOCM_RTS 0x004U
+#define TIOCM_ST 0x008U
+#define TIOCM_SR 0x010U
+#define TIOCM_CTS 0x020U
+#define TIOCM_CAR 0x040U
+#define TIOCM_RNG 0x080U
+#define TIOCM_DSR 0x100U
#define TIOCM_CD TIOCM_CAR
#define TIOCM_RI TIOCM_RNG
-#define TIOCM_OUT1 0x2000
-#define TIOCM_OUT2 0x4000
-#define TIOCM_LOOP 0x8000
+#define TIOCM_OUT1 0x2000U
+#define TIOCM_OUT2 0x4000U
+#define TIOCM_LOOP 0x8000U
/* ioctl (fd, TIOCSERGETLSR, &result) where result may be as below */
-#define TIOCSER_TEMT 0x01 /* Transmitter physically empty */
+#define TIOCSER_TEMT 0x01U /* Transmitter physically empty */
/* tcsetattr uses these */
#define TCSANOW 0
diff --git a/include/uapi/asm-generic/termbits.h b/include/uapi/asm-generic/termbits.h
index 890ef29053e2..10e01c77d2a6 100644
--- a/include/uapi/asm-generic/termbits.h
+++ b/include/uapi/asm-generic/termbits.h
@@ -58,88 +58,88 @@ struct ktermios {
#define VEOL2 16
/* c_iflag bits */
-#define IUCLC 0x0200
-#define IXON 0x0400
-#define IXOFF 0x1000
-#define IMAXBEL 0x2000
-#define IUTF8 0x4000
+#define IUCLC 0x0200U
+#define IXON 0x0400U
+#define IXOFF 0x1000U
+#define IMAXBEL 0x2000U
+#define IUTF8 0x4000U
/* c_oflag bits */
-#define OLCUC 0x00002
-#define ONLCR 0x00004
-#define NLDLY 0x00100
-#define NL0 0x00000
-#define NL1 0x00100
-#define CRDLY 0x00600
-#define CR0 0x00000
-#define CR1 0x00200
-#define CR2 0x00400
-#define CR3 0x00600
-#define TABDLY 0x01800
-#define TAB0 0x00000
-#define TAB1 0x00800
-#define TAB2 0x01000
-#define TAB3 0x01800
-#define XTABS 0x01800
-#define BSDLY 0x02000
-#define BS0 0x00000
-#define BS1 0x02000
-#define VTDLY 0x04000
-#define VT0 0x00000
-#define VT1 0x04000
-#define FFDLY 0x08000
-#define FF0 0x00000
-#define FF1 0x08000
+#define OLCUC 0x00002U
+#define ONLCR 0x00004U
+#define NLDLY 0x00100U
+#define NL0 0x00000U
+#define NL1 0x00100U
+#define CRDLY 0x00600U
+#define CR0 0x00000U
+#define CR1 0x00200U
+#define CR2 0x00400U
+#define CR3 0x00600U
+#define TABDLY 0x01800U
+#define TAB0 0x00000U
+#define TAB1 0x00800U
+#define TAB2 0x01000U
+#define TAB3 0x01800U
+#define XTABS 0x01800U
+#define BSDLY 0x02000U
+#define BS0 0x00000U
+#define BS1 0x02000U
+#define VTDLY 0x04000U
+#define VT0 0x00000U
+#define VT1 0x04000U
+#define FFDLY 0x08000U
+#define FF0 0x00000U
+#define FF1 0x08000U
/* c_cflag bit meaning */
-#define CBAUD 0x0000100f
-#define CSIZE 0x00000030
-#define CS5 0x00000000
-#define CS6 0x00000010
-#define CS7 0x00000020
-#define CS8 0x00000030
-#define CSTOPB 0x00000040
-#define CREAD 0x00000080
-#define PARENB 0x00000100
-#define PARODD 0x00000200
-#define HUPCL 0x00000400
-#define CLOCAL 0x00000800
-#define CBAUDEX 0x00001000
-#define BOTHER 0x00001000
-#define B57600 0x00001001
-#define B115200 0x00001002
-#define B230400 0x00001003
-#define B460800 0x00001004
-#define B500000 0x00001005
-#define B576000 0x00001006
-#define B921600 0x00001007
-#define B1000000 0x00001008
-#define B1152000 0x00001009
-#define B1500000 0x0000100a
-#define B2000000 0x0000100b
-#define B2500000 0x0000100c
-#define B3000000 0x0000100d
-#define B3500000 0x0000100e
-#define B4000000 0x0000100f
-#define CIBAUD 0x100f0000 /* input baud rate */
+#define CBAUD 0x0000100fU
+#define CSIZE 0x00000030U
+#define CS5 0x00000000U
+#define CS6 0x00000010U
+#define CS7 0x00000020U
+#define CS8 0x00000030U
+#define CSTOPB 0x00000040U
+#define CREAD 0x00000080U
+#define PARENB 0x00000100U
+#define PARODD 0x00000200U
+#define HUPCL 0x00000400U
+#define CLOCAL 0x00000800U
+#define CBAUDEX 0x00001000U
+#define BOTHER 0x00001000U
+#define B57600 0x00001001U
+#define B115200 0x00001002U
+#define B230400 0x00001003U
+#define B460800 0x00001004U
+#define B500000 0x00001005U
+#define B576000 0x00001006U
+#define B921600 0x00001007U
+#define B1000000 0x00001008U
+#define B1152000 0x00001009U
+#define B1500000 0x0000100aU
+#define B2000000 0x0000100bU
+#define B2500000 0x0000100cU
+#define B3000000 0x0000100dU
+#define B3500000 0x0000100eU
+#define B4000000 0x0000100fU
+#define CIBAUD 0x100f0000U /* input baud rate */
/* c_lflag bits */
-#define ISIG 0x00001
-#define ICANON 0x00002
-#define XCASE 0x00004
-#define ECHO 0x00008
-#define ECHOE 0x00010
-#define ECHOK 0x00020
-#define ECHONL 0x00040
-#define NOFLSH 0x00080
-#define TOSTOP 0x00100
-#define ECHOCTL 0x00200
-#define ECHOPRT 0x00400
-#define ECHOKE 0x00800
-#define FLUSHO 0x01000
-#define PENDIN 0x04000
-#define IEXTEN 0x08000
-#define EXTPROC 0x10000
+#define ISIG 0x00001U
+#define ICANON 0x00002U
+#define XCASE 0x00004U
+#define ECHO 0x00008U
+#define ECHOE 0x00010U
+#define ECHOK 0x00020U
+#define ECHONL 0x00040U
+#define NOFLSH 0x00080U
+#define TOSTOP 0x00100U
+#define ECHOCTL 0x00200U
+#define ECHOPRT 0x00400U
+#define ECHOKE 0x00800U
+#define FLUSHO 0x01000U
+#define PENDIN 0x04000U
+#define IEXTEN 0x08000U
+#define EXTPROC 0x10000U
/* tcsetattr uses these */
#define TCSANOW 0
base-commit: 83a7eefedc9b56fe7bfeff13b6c7356688ffa670
--
2.45.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: termios constants should be unsigned
From: Greg KH @ 2024-06-12 12:22 UTC (permalink / raw)
To: Alejandro Colomar
Cc: Andrew Morton, Palmer Dabbelt, linux-api, libc-alpha, linux-man
In-Reply-To: <a7kfppfptkzvqys6cblwjudlpoghsycjglw57hxe2ywvruzkbd@e6nqpnxgwfnq>
On Wed, Jun 12, 2024 at 02:16:12PM +0200, Alejandro Colomar wrote:
> Hi!
>
> While compiling an example program in ioctl_tty(2) examples (now moving
> to TCSETS(2const)), I saw several warnings:
>
> tcgets.c:53:24: error: implicit conversion changes signedness: 'int' to 'tcflag_t' (aka 'unsigned int') [clang-diagnostic-sign-conversion,-warnings-as-errors]
> tio.c_cflag &= ~CBAUD;
> ~~ ^~~~~~
> tcgets.c:53:24: warning: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise]
> tio.c_cflag &= ~CBAUD;
> ~~ ^~~~~~
> tcgets.c:58:24: error: implicit conversion changes signedness: 'int' to 'tcflag_t' (aka 'unsigned int') [clang-diagnostic-sign-conversion,-warnings-as-errors]
> tio.c_cflag &= ~(CBAUD << IBSHIFT);
> ~~ ^~~~~~~~~~~~~~~~~~~
> tcgets.c:58:24: warning: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise]
> tio.c_cflag &= ~(CBAUD << IBSHIFT);
> ~~ ^~~~~~~~~~~~~~~~~~~
> tcgets.c:58:25: warning: use of a signed integer operand with a unary bitwise operator [hicpp-signed-bitwise]
> tio.c_cflag &= ~(CBAUD << IBSHIFT);
> ~^~~~~~~~~~~~~~~~~~
>
> Which seem reasonable warnings. Bitwise operations on signed integers
> is bad for ye health. Let's see the definitions of struct termios:
>
> alx@debian:/usr/include$ grepc termios x86_64-linux-gnu/ asm-generic/
> x86_64-linux-gnu/bits/termios-struct.h:struct termios
> {
> tcflag_t c_iflag; /* input mode flags */
> tcflag_t c_oflag; /* output mode flags */
> tcflag_t c_cflag; /* control mode flags */
> tcflag_t c_lflag; /* local mode flags */
> cc_t c_line; /* line discipline */
> cc_t c_cc[NCCS]; /* control characters */
> speed_t c_ispeed; /* input speed */
> speed_t c_ospeed; /* output speed */
> #define _HAVE_STRUCT_TERMIOS_C_ISPEED 1
> #define _HAVE_STRUCT_TERMIOS_C_OSPEED 1
> };
> asm-generic/termbits.h:struct termios {
> tcflag_t c_iflag; /* input mode flags */
> tcflag_t c_oflag; /* output mode flags */
> tcflag_t c_cflag; /* control mode flags */
> tcflag_t c_lflag; /* local mode flags */
> cc_t c_line; /* line discipline */
> cc_t c_cc[NCCS]; /* control characters */
> };
>
> Except for the speed members, they seem the same. The types of the
> members are correctly unsigned:
>
> alx@debian:/usr/include$ grepc tcflag_t x86_64-linux-gnu/ asm-generic/
> x86_64-linux-gnu/bits/termios.h:typedef unsigned int tcflag_t;
> asm-generic/termbits.h:typedef unsigned int tcflag_t;
>
> So, all of the constants that are to be used in these members, which
> will undergo bitwise operations, should use the U suffix. Here's the
> list for the UAPI:
>
> $ sed -n '/flag bits/,/^$/p' asm-generic/termbits.h
> /* c_iflag bits */
> #define IUCLC 0x0200
> #define IXON 0x0400
> #define IXOFF 0x1000
> #define IMAXBEL 0x2000
> #define IUTF8 0x4000
>
> /* c_oflag bits */
> #define OLCUC 0x00002
> #define ONLCR 0x00004
> #define NLDLY 0x00100
> #define NL0 0x00000
> #define NL1 0x00100
> #define CRDLY 0x00600
> #define CR0 0x00000
> #define CR1 0x00200
> #define CR2 0x00400
> #define CR3 0x00600
> #define TABDLY 0x01800
> #define TAB0 0x00000
> #define TAB1 0x00800
> #define TAB2 0x01000
> #define TAB3 0x01800
> #define XTABS 0x01800
> #define BSDLY 0x02000
> #define BS0 0x00000
> #define BS1 0x02000
> #define VTDLY 0x04000
> #define VT0 0x00000
> #define VT1 0x04000
> #define FFDLY 0x08000
> #define FF0 0x00000
> #define FF1 0x08000
>
> /* c_lflag bits */
> #define ISIG 0x00001
> #define ICANON 0x00002
> #define XCASE 0x00004
> #define ECHO 0x00008
> #define ECHOE 0x00010
> #define ECHOK 0x00020
> #define ECHONL 0x00040
> #define NOFLSH 0x00080
> #define TOSTOP 0x00100
> #define ECHOCTL 0x00200
> #define ECHOPRT 0x00400
> #define ECHOKE 0x00800
> #define FLUSHO 0x01000
> #define PENDIN 0x04000
> #define IEXTEN 0x08000
> #define EXTPROC 0x10000
>
> And glibc also (re)defines some of them, so those should also have the
> U suffix.
>
> Any opinions?
Have a proposed patch that you feel would resolve this?
thanks,
greg k-h
^ 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