| Message ID | 20180618080618.495174114@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/18/2018 10:13 AM, Greg Kroah-Hartman wrote:
> 4.16-stable review patch. If anyone has any objections, please let me know.
So I was wondering, why backport such a considerable number of
*selftests* to stable, given the stable policy? Surely selftests don't
affect the kernel itself breaking for users?
Thanks, Vlastimil
On Tue, Jul 03, 2018 at 01:36:43PM +0200, Vlastimil Babka wrote: > On 06/18/2018 10:13 AM, Greg Kroah-Hartman wrote: > > 4.16-stable review patch. If anyone has any objections, please let me know. > > So I was wondering, why backport such a considerable number of > *selftests* to stable, given the stable policy? Surely selftests don't > affect the kernel itself breaking for users? These came in as part of Sasha's "backport fixes" tool. It can't hurt to add selftest fixes/updates to stable kernels, as for some people, they only run the selftests for the specific kernel they are building. While others run selftests for the latest kernel on older kernels, both of which are valid ways of testing. thanks, greg k-h
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > On Tue, Jul 03, 2018 at 01:36:43PM +0200, Vlastimil Babka wrote: >> On 06/18/2018 10:13 AM, Greg Kroah-Hartman wrote: >> > 4.16-stable review patch. If anyone has any objections, please let me know. >> >> So I was wondering, why backport such a considerable number of >> *selftests* to stable, given the stable policy? Surely selftests don't >> affect the kernel itself breaking for users? > > These came in as part of Sasha's "backport fixes" tool. It can't hurt > to add selftest fixes/updates to stable kernels, as for some people, > they only run the selftests for the specific kernel they are building. > While others run selftests for the latest kernel on older kernels, both > of which are valid ways of testing. I don't have a problem with these sort of patches being backported, but it seems like Documentation/process/stable-kernel-rules.txt could use an update? I honestly don't know what the rules are anymore. cheers
* Michael Ellerman <mpe@ellerman.id.au> wrote: > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > > > On Tue, Jul 03, 2018 at 01:36:43PM +0200, Vlastimil Babka wrote: > >> On 06/18/2018 10:13 AM, Greg Kroah-Hartman wrote: > >> > 4.16-stable review patch. If anyone has any objections, please let me know. > >> > >> So I was wondering, why backport such a considerable number of > >> *selftests* to stable, given the stable policy? Surely selftests don't > >> affect the kernel itself breaking for users? > > > > These came in as part of Sasha's "backport fixes" tool. It can't hurt > > to add selftest fixes/updates to stable kernels, as for some people, > > they only run the selftests for the specific kernel they are building. > > While others run selftests for the latest kernel on older kernels, both > > of which are valid ways of testing. > > I don't have a problem with these sort of patches being backported, but > it seems like Documentation/process/stable-kernel-rules.txt could use an > update? > > I honestly don't know what the rules are anymore. Self-tests are standalone tooling which help the testing of the kernel, and it makes sense to either update all of them, or none of them. Here it makes sense to update all of them, because if a self-test on a stable kernel shows a failure then a fix is probably missing from -stable, right? Also note that self-test tooling *cannot possibly break the kernel*, because they are not used in the kernel build process, so the normally conservative backporting rules do not apply. Thanks, Ingo
Ingo Molnar <mingo@kernel.org> writes: > * Michael Ellerman <mpe@ellerman.id.au> wrote: >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: >> > On Tue, Jul 03, 2018 at 01:36:43PM +0200, Vlastimil Babka wrote: >> >> On 06/18/2018 10:13 AM, Greg Kroah-Hartman wrote: >> >> > 4.16-stable review patch. If anyone has any objections, please let me know. >> >> >> >> So I was wondering, why backport such a considerable number of >> >> *selftests* to stable, given the stable policy? Surely selftests don't >> >> affect the kernel itself breaking for users? >> > >> > These came in as part of Sasha's "backport fixes" tool. It can't hurt >> > to add selftest fixes/updates to stable kernels, as for some people, >> > they only run the selftests for the specific kernel they are building. >> > While others run selftests for the latest kernel on older kernels, both >> > of which are valid ways of testing. >> >> I don't have a problem with these sort of patches being backported, but >> it seems like Documentation/process/stable-kernel-rules.txt could use an >> update? >> >> I honestly don't know what the rules are anymore. > > Self-tests are standalone tooling which help the testing of the kernel, and it > makes sense to either update all of them, or none of them. Yes I know what selftests are. > Here it makes sense to update all of them, because if a self-test on a stable > kernel shows a failure then a fix is probably missing from -stable, right? Usually, though it's not always that simple IME. But sure, I don't have a problem with updating selftests, I said that before. > Also note that self-test tooling *cannot possibly break the kernel*, because they > are not used in the kernel build process, so the normally conservative backporting > rules do not apply. Right. So stable-kernel-rules.txt could use an update to mention that. My comment was less about this actual patch and more about the new reality of patches being backported to stable based on Sasha's tooling, which seems to be much more liberal than anything we've done previously. I don't generally have any objection to that process, though it possibly could have been more widely announced. But, it would be good if stable-kernel-rules.txt was updated to mention it. I've had several people ask me "hey my patch got backported to stable but I didn't ask for it - is that OK, what's going on?" etc. I guess I should just send a patch to update it, but I don't really know what it should say. cheers
On Sun, Jul 08, 2018 at 08:33:37PM +1000, Michael Ellerman wrote: > Ingo Molnar <mingo@kernel.org> writes: > > * Michael Ellerman <mpe@ellerman.id.au> wrote: > >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > >> > On Tue, Jul 03, 2018 at 01:36:43PM +0200, Vlastimil Babka wrote: > >> >> On 06/18/2018 10:13 AM, Greg Kroah-Hartman wrote: > >> >> > 4.16-stable review patch. If anyone has any objections, please let me know. > >> >> > >> >> So I was wondering, why backport such a considerable number of > >> >> *selftests* to stable, given the stable policy? Surely selftests don't > >> >> affect the kernel itself breaking for users? > >> > > >> > These came in as part of Sasha's "backport fixes" tool. It can't hurt > >> > to add selftest fixes/updates to stable kernels, as for some people, > >> > they only run the selftests for the specific kernel they are building. > >> > While others run selftests for the latest kernel on older kernels, both > >> > of which are valid ways of testing. > >> > >> I don't have a problem with these sort of patches being backported, but > >> it seems like Documentation/process/stable-kernel-rules.txt could use an > >> update? > >> > >> I honestly don't know what the rules are anymore. > > > > Self-tests are standalone tooling which help the testing of the kernel, and it > > makes sense to either update all of them, or none of them. > > Yes I know what selftests are. > > > Here it makes sense to update all of them, because if a self-test on a stable > > kernel shows a failure then a fix is probably missing from -stable, right? > > Usually, though it's not always that simple IME. > > But sure, I don't have a problem with updating selftests, I said that before. > > > Also note that self-test tooling *cannot possibly break the kernel*, because they > > are not used in the kernel build process, so the normally conservative backporting > > rules do not apply. > > Right. So stable-kernel-rules.txt could use an update to mention that. > > > My comment was less about this actual patch and more about the new > reality of patches being backported to stable based on Sasha's tooling, > which seems to be much more liberal than anything we've done previously. > > I don't generally have any objection to that process, though it possibly > could have been more widely announced. But, it would be good if > stable-kernel-rules.txt was updated to mention it. > > I've had several people ask me "hey my patch got backported to stable > but I didn't ask for it - is that OK, what's going on?" etc. Why didn't those people just ask us? To not do so is very strange, it's not like we are hard to find :) > I guess I should just send a patch to update it, but I don't really know > what it should say. I don't think it really needs any changes, as the selftests is just a corner case that is easily explained if anyone cares enough to actually ask :) thanks, greg k-h
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > On Sun, Jul 08, 2018 at 08:33:37PM +1000, Michael Ellerman wrote: ... >> >> My comment was less about this actual patch and more about the new >> reality of patches being backported to stable based on Sasha's tooling, >> which seems to be much more liberal than anything we've done previously. >> >> I don't generally have any objection to that process, though it possibly >> could have been more widely announced. But, it would be good if >> stable-kernel-rules.txt was updated to mention it. >> >> I've had several people ask me "hey my patch got backported to stable >> but I didn't ask for it - is that OK, what's going on?" etc. > > Why didn't those people just ask us? To not do so is very strange, it's > not like we are hard to find :) It's not very strange, it's completely normal behaviour. People are afraid of asking dumb questions in public, so they ask someone privately. And the general sentiment has been "I didn't think that patch met the stable rules, but I'm happy for it to be backported". >> I guess I should just send a patch to update it, but I don't really know >> what it should say. > > I don't think it really needs any changes, as the selftests is just a > corner case that is easily explained if anyone cares enough to actually > ask :) Yeah again I'm not really concerned about selftests, I should have replied to a different patch to start this discussion. My bad. cheers
--- a/tools/testing/selftests/x86/protection_keys.c +++ b/tools/testing/selftests/x86/protection_keys.c @@ -191,26 +191,30 @@ void lots_o_noops_around_write(int *writ #ifdef __i386__ #ifndef SYS_mprotect_key -# define SYS_mprotect_key 380 +# define SYS_mprotect_key 380 #endif + #ifndef SYS_pkey_alloc -# define SYS_pkey_alloc 381 -# define SYS_pkey_free 382 +# define SYS_pkey_alloc 381 +# define SYS_pkey_free 382 #endif -#define REG_IP_IDX REG_EIP -#define si_pkey_offset 0x14 + +#define REG_IP_IDX REG_EIP +#define si_pkey_offset 0x14 #else #ifndef SYS_mprotect_key -# define SYS_mprotect_key 329 +# define SYS_mprotect_key 329 #endif + #ifndef SYS_pkey_alloc -# define SYS_pkey_alloc 330 -# define SYS_pkey_free 331 +# define SYS_pkey_alloc 330 +# define SYS_pkey_free 331 #endif -#define REG_IP_IDX REG_RIP -#define si_pkey_offset 0x20 + +#define REG_IP_IDX REG_RIP +#define si_pkey_offset 0x20 #endif @@ -225,8 +229,14 @@ void dump_mem(void *dumpme, int len_byte } } -#define SEGV_BNDERR 3 /* failed address bound checks */ -#define SEGV_PKUERR 4 +/* Failed address bound checks: */ +#ifndef SEGV_BNDERR +# define SEGV_BNDERR 3 +#endif + +#ifndef SEGV_PKUERR +# define SEGV_PKUERR 4 +#endif static char *si_code_str(int si_code) { @@ -393,10 +403,15 @@ pid_t fork_lazy_child(void) return forkret; } -#define PKEY_DISABLE_ACCESS 0x1 -#define PKEY_DISABLE_WRITE 0x2 +#ifndef PKEY_DISABLE_ACCESS +# define PKEY_DISABLE_ACCESS 0x1 +#endif + +#ifndef PKEY_DISABLE_WRITE +# define PKEY_DISABLE_WRITE 0x2 +#endif -u32 pkey_get(int pkey, unsigned long flags) +static u32 hw_pkey_get(int pkey, unsigned long flags) { u32 mask = (PKEY_DISABLE_ACCESS|PKEY_DISABLE_WRITE); u32 pkru = __rdpkru(); @@ -418,7 +433,7 @@ u32 pkey_get(int pkey, unsigned long fla return masked_pkru; } -int pkey_set(int pkey, unsigned long rights, unsigned long flags) +static int hw_pkey_set(int pkey, unsigned long rights, unsigned long flags) { u32 mask = (PKEY_DISABLE_ACCESS|PKEY_DISABLE_WRITE); u32 old_pkru = __rdpkru(); @@ -452,15 +467,15 @@ void pkey_disable_set(int pkey, int flag pkey, flags); pkey_assert(flags & (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE)); - pkey_rights = pkey_get(pkey, syscall_flags); + pkey_rights = hw_pkey_get(pkey, syscall_flags); - dprintf1("%s(%d) pkey_get(%d): %x\n", __func__, + dprintf1("%s(%d) hw_pkey_get(%d): %x\n", __func__, pkey, pkey, pkey_rights); pkey_assert(pkey_rights >= 0); pkey_rights |= flags; - ret = pkey_set(pkey, pkey_rights, syscall_flags); + ret = hw_pkey_set(pkey, pkey_rights, syscall_flags); assert(!ret); /*pkru and flags have the same format */ shadow_pkru |= flags << (pkey * 2); @@ -468,8 +483,8 @@ void pkey_disable_set(int pkey, int flag pkey_assert(ret >= 0); - pkey_rights = pkey_get(pkey, syscall_flags); - dprintf1("%s(%d) pkey_get(%d): %x\n", __func__, + pkey_rights = hw_pkey_get(pkey, syscall_flags); + dprintf1("%s(%d) hw_pkey_get(%d): %x\n", __func__, pkey, pkey, pkey_rights); dprintf1("%s(%d) pkru: 0x%x\n", __func__, pkey, rdpkru()); @@ -483,24 +498,24 @@ void pkey_disable_clear(int pkey, int fl { unsigned long syscall_flags = 0; int ret; - int pkey_rights = pkey_get(pkey, syscall_flags); + int pkey_rights = hw_pkey_get(pkey, syscall_flags); u32 orig_pkru = rdpkru(); pkey_assert(flags & (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE)); - dprintf1("%s(%d) pkey_get(%d): %x\n", __func__, + dprintf1("%s(%d) hw_pkey_get(%d): %x\n", __func__, pkey, pkey, pkey_rights); pkey_assert(pkey_rights >= 0); pkey_rights |= flags; - ret = pkey_set(pkey, pkey_rights, 0); + ret = hw_pkey_set(pkey, pkey_rights, 0); /* pkru and flags have the same format */ shadow_pkru &= ~(flags << (pkey * 2)); pkey_assert(ret >= 0); - pkey_rights = pkey_get(pkey, syscall_flags); - dprintf1("%s(%d) pkey_get(%d): %x\n", __func__, + pkey_rights = hw_pkey_get(pkey, syscall_flags); + dprintf1("%s(%d) hw_pkey_get(%d): %x\n", __func__, pkey, pkey, pkey_rights); dprintf1("%s(%d) pkru: 0x%x\n", __func__, pkey, rdpkru());