On Mon, 27 Apr 2026 at 10:18, Christophe Leroy (CS GROUP)
<[email protected]> wrote:
>
> In a subsequent patch, copy_{to/from}_user() will be modified to
> return -EFAULT when copy fails.
Please don't do this.
This is a maintenance nightmare, and changes pretty much three decades
of semantics, and will cause *very* subtle backporting issues if
somebody happens to rely on the old / new behavior.
I understand the reasoning for the change, but I really don't think
the pain of creating yet another user copy interface is worth it.
We already have a lot of different versions of user copies for
different reasons, and while they all tend to have a good reason (and
some not-so-good, but historical reasons) for existing, this one
doesn't seem worth it.
The main - perhaps only - reason for this "partial" version is that
you want to do that "automatically inlined and optimized fixed-sized
case".
But here's the thing: I think you can already do that. Yes, it
requires some improvements to unsafe_copy_from_user(), but *that*
interface doesn't have three decades of history associated with it,
_and_ you're extending on that one anyway in this series.
"unsafe_copy_from_user()" is very odd, is meant only for small simple
copies that can be inlined and it's special-cased for 'objtool' anyway
(because objtool would have complained about an out-of-line call,
although it could have been special-cased other ways).
In other words: unsafe_copy_from_user() is *very* close to what you
want for that "Oh, I noticed that it's a small fixed-size copy, so I
want to special-case copy-from-user for that".
The _only_ issue with unsafe_copy_from_user() is that you can't see
that there were partial successes. But if *that* was fixed, then this
whole "create a new copy_from_user interface" issue would just go
away.
So please - let's just change unsafe_copy_from_user() to be usable for
the partial case.
And the thing is, all the existing unsafe_copy_from_user()
implementations already effectively *have* the "how much did I not
copy" internally, and they actually do extra work to hide it, ie they
have things like that
int _i;
that is "how many bytes have I copied" in the powerpc implementation,
or the x86 code does
size_t __ucu_len = (_len);
where that "ucu_len" is updated as you go along and is literally the
"how many bytes are left to copy" return value that is missing from
this interface.
So what I would suggest is
- introduce a new user accessor helper that is used for *both*
unsafe_copy_to/from_user() *and* the "inline small constant-sized
normal copy_to/from_user()" calls
- it's the same thing as the existing unsafe_copy_to/from_user()
implementation, except it exposes how many bytes are left to be copied
to the exception label.
IOW, it would look something like
#define unsafe_copy_to_user_outlen(_dst,_src,_len,label)...
which is exactly the same as the current unsafe_copy_to_user(),
*except* it changes "_len" as it does along.
And then you use that for both the "real" unsafe_copy_user and for the
"small constant values" case.
Just as an example, attached is a completely stupid rough draft of a
patch that does this for x86 and only for unsafe_copy_to_user().
And I made a very very hacky change to kernel/sys.c to see what the
code generation looks like.
This is what it results in on x86 with clang (with all the magic
.section data edited out):
... edited out the code to generate the times
... this is the actual user copy:
# HERE!
movabsq $81985529216486895, %rcx # imm = 0x123456789ABCDEF
cmpq %rcx, %rbx
cmovaq %rcx, %rbx
stac
movq %r13, (%rbx) # exception to .LBB45_8
movq %r14, 8(%rbx) # exception to .LBB45_8
movq %r15, 16(%rbx) # exception to .LBB45_8
movq %rax, 24(%rbx) # exception to .LBB45_8
clac
.LBB45_6:
movq jiffies(%rip), %rdi
callq jiffies_64_to_clock_t
.LBB45_7:
addq $16, %rsp
popq %rbx
popq %r12
popq %r13
popq %r14
popq %r15
retq
.LBB45_8:
clac
movq $-14, %rax
jmp .LBB45_7
and notice how the compiler noticed that the 'outlen' isn't actually
used, and turned the exception label into just a "return -EFAULT" and
never actually generated any code for updating remaining lengths?
That actually looks pretty much optimal for a 32-byte user copy.
And it didn't involve changing the semantics at all.
Just to check, I changed that "times()" system call to return the
number of bytes uncopied instead (to emulate the "I actually want to
know what's left" case), and it generated this:
# HERE!
movabsq $81985529216486895, %rcx # imm = 0x123456789ABCDEF
cmpq %rcx, %rbx
cmovaq %rcx, %rbx
stac
movl $32, %ecx
movq %r13, (%rbx) # exception to .LBB45_7
movl $24, %ecx
movq %r15, 8(%rbx) # exception to .LBB45_7
movl $16, %ecx
movq %r14, 16(%rbx) # exception to .LBB45_7
movl $8, %ecx
movq %rax, 24(%rbx) # exception to .LBB45_7
clac
xorl %ecx, %ecx
.LBB45_8:
movq %rcx, %rax
addq $16, %rsp
popq %rbx
popq %r12
popq %r13
popq %r14
popq %r15
retq
.LBB45_6:
movq jiffies(%rip), %rdi
jmp jiffies_64_to_clock_t # TAILCALL
.LBB45_7:
clac
jmp .LBB45_8
so it all seems to work - although obviously the above is *not* the normal case.
NOTE NOTE NOTE! The attached patch is entirely untested. I obviously
did some "test code generation" with it, but I only *looked* at the
result, and maybe it has some fundamental problem that I just didn't
notice. So treat this as a "how about this approach" patch, not as
anything more serious than that.
And the kerrnel/sys.c hack is very obviously just that: a complate
hack for testing.
A real patch would do that "for small constant-sized copies, turn
copy_to_user() automatically into "_small_copy_to_user()".
The attached is *not* a real patch. Treat it with the contempt it deserves.
Linus
arch/x86/include/asm/uaccess.h | 17 +++++++++++------
include/linux/uaccess.h | 16 ++++++++++++++++
kernel/sys.c | 3 ++-
3 files changed, 29 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 3a0dd3c2b233..3b2c57c91418 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -606,15 +606,20 @@ _label: \
len -= sizeof(type); \
}
-#define unsafe_copy_to_user(_dst,_src,_len,label) \
+#define unsafe_copy_to_user_outlen(_dst,_src,_len,label) \
do { \
char __user *__ucu_dst = (_dst); \
const char *__ucu_src = (_src); \
- size_t __ucu_len = (_len); \
- unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u64, label); \
- unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u32, label); \
- unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u16, label); \
- unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u8, label); \
+ unsafe_copy_loop(__ucu_dst, __ucu_src, _len, u64, label); \
+ unsafe_copy_loop(__ucu_dst, __ucu_src, _len, u32, label); \
+ unsafe_copy_loop(__ucu_dst, __ucu_src, _len, u16, label); \
+ unsafe_copy_loop(__ucu_dst, __ucu_src, _len, u8, label); \
+} while (0)
+
+#define unsafe_copy_to_user(_dst,_src,_len,label) \
+do { \
+ size_t __ucu_len = _len; \
+ unsafe_copy_to_user_outlen(_dst,_src,__ucu_len,label); \
} while (0)
#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 56328601218c..1a70ef70784c 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -874,4 +874,20 @@ void __noreturn usercopy_abort(const char *name, const char *detail,
unsigned long len);
#endif
+static __always_inline __must_check unsigned long
+_small_copy_to_user(void __user *to, const void *from, unsigned long n)
+{
+ size_t uncopied = n;
+
+ might_fault();
+ if (should_fail_usercopy())
+ return n;
+ instrument_copy_to_user(to, from, n);
+ scoped_user_write_access_size(to, n, failed)
+ unsafe_copy_to_user_outlen(to, from, uncopied, failed);
+ return 0;
+failed:
+ return uncopied;
+}
+
#endif /* __LINUX_UACCESS_H__ */
diff --git a/kernel/sys.c b/kernel/sys.c
index 62e842055cc9..65b2d0103a73 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1067,7 +1067,8 @@ SYSCALL_DEFINE1(times, struct tms __user *, tbuf)
struct tms tmp;
do_sys_times(&tmp);
- if (copy_to_user(tbuf, &tmp, sizeof(struct tms)))
+ asm volatile("# HERE!");
+ if (_small_copy_to_user(tbuf, &tmp, sizeof(struct tms)))
return -EFAULT;
}
force_successful_syscall_return();
