| Message ID | 20220214163452.1568807-4-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clean up asm/uaccess.h, kill set_fs for good | expand |
On Mon, Feb 14, 2022 at 05:34:41PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The get_user()/put_user() functions are meant to check for > access_ok(), while the __get_user()/__put_user() functions > don't. > > This broke in 4.19 for nds32, when it gained an extraneous > check in __get_user(), but lost the check it needs in > __put_user(). Can we follow the lead of MIPS (which this was originally copied from I think) and kill the pointless __get/put_user_check wrapper that just obsfucate the code?
From: Christoph Hellwig > Sent: 14 February 2022 17:01 > > On Mon, Feb 14, 2022 at 05:34:41PM +0100, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > The get_user()/put_user() functions are meant to check for > > access_ok(), while the __get_user()/__put_user() functions > > don't. > > > > This broke in 4.19 for nds32, when it gained an extraneous > > check in __get_user(), but lost the check it needs in > > __put_user(). > > Can we follow the lead of MIPS (which this was originally copied > from I think) and kill the pointless __get/put_user_check wrapper > that just obsfucate the code? Is it possible to make all these architectures fall back to a common definition somewhere? Maybe they need to define ACCESS_OK_USER_LIMIT - which can be different from TASK_SIZE. There'll be a few special cases, but most architectures have kernel addresses above userspace ones. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Feb 14, 2022 at 6:01 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Mon, Feb 14, 2022 at 05:34:41PM +0100, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > The get_user()/put_user() functions are meant to check for > > access_ok(), while the __get_user()/__put_user() functions > > don't. > > > > This broke in 4.19 for nds32, when it gained an extraneous > > check in __get_user(), but lost the check it needs in > > __put_user(). > > Can we follow the lead of MIPS (which this was originally copied > from I think) and kill the pointless __get/put_user_check wrapper > that just obsfucate the code? I had another look, but I think that would be a bigger change than I want to have in a fix for stable backports, as nds32 also uses the _check versions in __{get,put}_user_error. If we instead clean it up in a separate patch, it should be done for all eight architectures that do the same thing, but at that point, the time seems better spent at coming up with a new set of calling conventions that work with asm-goto. Arnd
On Tue, Feb 15, 2022 at 10:18:15AM +0100, Arnd Bergmann wrote: > On Mon, Feb 14, 2022 at 6:01 PM Christoph Hellwig <hch@infradead.org> wrote: > > > > On Mon, Feb 14, 2022 at 05:34:41PM +0100, Arnd Bergmann wrote: > > > From: Arnd Bergmann <arnd@arndb.de> > > > > > > The get_user()/put_user() functions are meant to check for > > > access_ok(), while the __get_user()/__put_user() functions > > > don't. > > > > > > This broke in 4.19 for nds32, when it gained an extraneous > > > check in __get_user(), but lost the check it needs in > > > __put_user(). > > > > Can we follow the lead of MIPS (which this was originally copied > > from I think) and kill the pointless __get/put_user_check wrapper > > that just obsfucate the code? > > I had another look, but I think that would be a bigger change than > I want to have in a fix for stable backports, as nds32 also uses > the _check versions in __{get,put}_user_error. Don't worry about stable backports first, get it correct and merged and then worry about them if you really have to. If someone cares about nds32 for stable kernels, they can do the backport work :) thanks, greg k-h
diff --git a/arch/nds32/include/asm/uaccess.h b/arch/nds32/include/asm/uaccess.h index d4cbf069dc22..37a40981deb3 100644 --- a/arch/nds32/include/asm/uaccess.h +++ b/arch/nds32/include/asm/uaccess.h @@ -70,9 +70,7 @@ static inline void set_fs(mm_segment_t fs) * versions are void (ie, don't return a value as such). */ -#define get_user __get_user \ - -#define __get_user(x, ptr) \ +#define get_user(x, ptr) \ ({ \ long __gu_err = 0; \ __get_user_check((x), (ptr), __gu_err); \ @@ -85,6 +83,14 @@ static inline void set_fs(mm_segment_t fs) (void)0; \ }) +#define __get_user(x, ptr) \ +({ \ + long __gu_err = 0; \ + const __typeof__(*(ptr)) __user *__p = (ptr); \ + __get_user_err((x), __p, (__gu_err)); \ + __gu_err; \ +}) + #define __get_user_check(x, ptr, err) \ ({ \ const __typeof__(*(ptr)) __user *__p = (ptr); \ @@ -165,12 +171,18 @@ do { \ : "r"(addr), "i"(-EFAULT) \ : "cc") -#define put_user __put_user \ +#define put_user(x, ptr) \ +({ \ + long __pu_err = 0; \ + __put_user_check((x), (ptr), __pu_err); \ + __pu_err; \ +}) #define __put_user(x, ptr) \ ({ \ long __pu_err = 0; \ - __put_user_err((x), (ptr), __pu_err); \ + __typeof__(*(ptr)) __user *__p = (ptr); \ + __put_user_err((x), __p, __pu_err); \ __pu_err; \ })