* [PATCH] mm: Make failslab writable again
@ 2022-09-20 8:20 Alexander Atanasov
2022-09-20 8:42 ` Vlastimil Babka
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Atanasov @ 2022-09-20 8:20 UTC (permalink / raw)
To: Jonathan Corbet, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin,
Hyeonggon Yoo
Cc: kernel, Alexander Atanasov, Kees Cook, Roman Gushchin, Jann Horn,
Vijayanand Jitta, linux-doc, linux-kernel, linux-mm
In (060807f841ac mm, slub: make remaining slub_debug related attributes
read-only failslab) it was made RO.
I think it became a collateral victim to the other two options
(sanity_checks and trace) for which the reasons are perfectly valid.
Here is why:
- sanity_checks and trace are slab internal debug options,
failslab is used for fault injection.
- for fault injections, which by presumption are random, it
does not matter if it is not set atomically. You need to
set atleast one more option to trigger fault injection.
- in a testing scenario you may need to change it at runtime
example: module loading - you test all allocations limited
by the space option. Then you move to test only your module's
own slabs.
- when set by command line flags it effectively disables all
cache merges.
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Roman Gushchin <guro@fb.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Jann Horn <jannh@google.com>
Cc: Vijayanand Jitta <vjitta@codeaurora.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Pekka Enberg <penberg@kernel.org>
Link: http://lkml.kernel.org/r/20200610163135.17364-5-vbabka@suse.cz
Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com>
---
Documentation/mm/slub.rst | 2 ++
mm/slub.c | 14 +++++++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/Documentation/mm/slub.rst b/Documentation/mm/slub.rst
index 43063ade737a..86837073a39e 100644
--- a/Documentation/mm/slub.rst
+++ b/Documentation/mm/slub.rst
@@ -116,6 +116,8 @@ options from the ``slub_debug`` parameter translate to the following files::
T trace
A failslab
+failslab file is writable, so writing 1 or 0 will enable or disable
+the option at runtime. Write returns -EINVAL if cache is an alias.
Careful with tracing: It may spew out lots of information and never stop if
used on the wrong slab.
diff --git a/mm/slub.c b/mm/slub.c
index 862dbd9af4f5..7c15d312e0fb 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5617,7 +5617,19 @@ static ssize_t failslab_show(struct kmem_cache *s, char *buf)
{
return sysfs_emit(buf, "%d\n", !!(s->flags & SLAB_FAILSLAB));
}
-SLAB_ATTR_RO(failslab);
+
+static ssize_t failslab_store(struct kmem_cache *s, const char *buf,
+ size_t length)
+{
+ if (s->refcount > 1)
+ return -EINVAL;
+
+ s->flags &= ~SLAB_FAILSLAB;
+ if (buf[0] == '1')
+ s->flags |= SLAB_FAILSLAB;
+ return length;
+}
+SLAB_ATTR(failslab);
#endif
static ssize_t shrink_show(struct kmem_cache *s, char *buf)
base-commit: 80e78fcce86de0288793a0ef0f6acf37656ee4cf
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: Make failslab writable again
2022-09-20 8:20 [PATCH] mm: Make failslab writable again Alexander Atanasov
@ 2022-09-20 8:42 ` Vlastimil Babka
2022-09-20 9:17 ` Alexander Atanasov
0 siblings, 1 reply; 6+ messages in thread
From: Vlastimil Babka @ 2022-09-20 8:42 UTC (permalink / raw)
To: Alexander Atanasov, Jonathan Corbet, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
Roman Gushchin, Hyeonggon Yoo
Cc: kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta,
linux-doc, linux-kernel, linux-mm
On 9/20/22 10:20, Alexander Atanasov wrote:
> In (060807f841ac mm, slub: make remaining slub_debug related attributes
> read-only failslab) it was made RO.
"read-only) failslab was made RO" ?
> I think it became a collateral victim to the other two options
> (sanity_checks and trace) for which the reasons are perfectly valid.
The commit also mentioned that modifying the flags is not protected in any
way, see below.
> Here is why:
> - sanity_checks and trace are slab internal debug options,
> failslab is used for fault injection.
> - for fault injections, which by presumption are random, it
> does not matter if it is not set atomically. You need to
> set atleast one more option to trigger fault injection.
> - in a testing scenario you may need to change it at runtime
> example: module loading - you test all allocations limited
> by the space option. Then you move to test only your module's
> own slabs.
> - when set by command line flags it effectively disables all
> cache merges.
>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Roman Gushchin <guro@fb.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Vijayanand Jitta <vjitta@codeaurora.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Link: http://lkml.kernel.org/r/20200610163135.17364-5-vbabka@suse.cz
>
> Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com>
> ---
> Documentation/mm/slub.rst | 2 ++
> mm/slub.c | 14 +++++++++++++-
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/mm/slub.rst b/Documentation/mm/slub.rst
> index 43063ade737a..86837073a39e 100644
> --- a/Documentation/mm/slub.rst
> +++ b/Documentation/mm/slub.rst
> @@ -116,6 +116,8 @@ options from the ``slub_debug`` parameter translate to the following files::
> T trace
> A failslab
>
> +failslab file is writable, so writing 1 or 0 will enable or disable
> +the option at runtime. Write returns -EINVAL if cache is an alias.
> Careful with tracing: It may spew out lots of information and never stop if
> used on the wrong slab.
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 862dbd9af4f5..7c15d312e0fb 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5617,7 +5617,19 @@ static ssize_t failslab_show(struct kmem_cache *s, char *buf)
> {
> return sysfs_emit(buf, "%d\n", !!(s->flags & SLAB_FAILSLAB));
> }
> -SLAB_ATTR_RO(failslab);
> +
> +static ssize_t failslab_store(struct kmem_cache *s, const char *buf,
> + size_t length)
> +{
> + if (s->refcount > 1)
> + return -EINVAL;
> +
> + s->flags &= ~SLAB_FAILSLAB;
> + if (buf[0] == '1')
> + s->flags |= SLAB_FAILSLAB;
Could we at least use a temporary variable to set up the final value and
then do a WRITE_ONCE() to s->flags, so the compiler is not allowed to do
some funky stuff? Assuming this is really the only place where we modify
s->flags during runtime, so we can't miss other updates due to RMW.
> + return length;
> +}
> +SLAB_ATTR(failslab);
> #endif
>
> static ssize_t shrink_show(struct kmem_cache *s, char *buf)
>
> base-commit: 80e78fcce86de0288793a0ef0f6acf37656ee4cf
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: Make failslab writable again
2022-09-20 8:42 ` Vlastimil Babka
@ 2022-09-20 9:17 ` Alexander Atanasov
2022-09-20 9:29 ` Vlastimil Babka
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Atanasov @ 2022-09-20 9:17 UTC (permalink / raw)
To: Vlastimil Babka, Jonathan Corbet, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
Roman Gushchin, Hyeonggon Yoo
Cc: kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta,
linux-doc, linux-kernel, linux-mm
Hello,
On 20.09.22 11:42, Vlastimil Babka wrote:
> On 9/20/22 10:20, Alexander Atanasov wrote:
>> In (060807f841ac mm, slub: make remaining slub_debug related attributes
>> read-only failslab) it was made RO.
>
> "read-only) failslab was made RO" ?
Yep.
>> I think it became a collateral victim to the other two options
>> (sanity_checks and trace) for which the reasons are perfectly valid.
>
> The commit also mentioned that modifying the flags is not protected in any
> way, see below.
Yes, indeed.
>> +static ssize_t failslab_store(struct kmem_cache *s, const char *buf,
>> + size_t length)
>> +{
>> + if (s->refcount > 1)
>> + return -EINVAL;
>> +
>> + s->flags &= ~SLAB_FAILSLAB;
>> + if (buf[0] == '1')
>> + s->flags |= SLAB_FAILSLAB;
>
> Could we at least use a temporary variable to set up the final value and
> then do a WRITE_ONCE() to s->flags, so the compiler is not allowed to do
> some funky stuff? Assuming this is really the only place where we modify
> s->flags during runtime, so we can't miss other updates due to RMW.
Since it is set or clear - instead of temporary variable and potentially
two writes and RMW issues i would suggest this:
+ if (buf[0] == '1')
+ s->flags |= SLAB_FAILSLAB;
+ else
+ s->flags &= ~SLAB_FAILSLAB;
If at some point more places need to modify the flags at runtime they
can switch to atomic bit ops.
--
Regards,
Alexander Atanasov
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: Make failslab writable again
2022-09-20 9:17 ` Alexander Atanasov
@ 2022-09-20 9:29 ` Vlastimil Babka
2022-09-20 10:21 ` Alexander Atanasov
0 siblings, 1 reply; 6+ messages in thread
From: Vlastimil Babka @ 2022-09-20 9:29 UTC (permalink / raw)
To: Alexander Atanasov, Jonathan Corbet, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
Roman Gushchin, Hyeonggon Yoo
Cc: kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta,
linux-doc, linux-kernel, linux-mm
On 9/20/22 11:17, Alexander Atanasov wrote:
> Hello,
>
> On 20.09.22 11:42, Vlastimil Babka wrote:
>> On 9/20/22 10:20, Alexander Atanasov wrote:
>>> In (060807f841ac mm, slub: make remaining slub_debug related attributes
>>> read-only failslab) it was made RO.
>>
>> "read-only) failslab was made RO" ?
>
> Yep.
>
>>> I think it became a collateral victim to the other two options
>>> (sanity_checks and trace) for which the reasons are perfectly valid.
>>
>> The commit also mentioned that modifying the flags is not protected in any
>> way, see below.
>
> Yes, indeed.
>
>>> +static ssize_t failslab_store(struct kmem_cache *s, const char *buf,
>>> + size_t length)
>>> +{
>>> + if (s->refcount > 1)
>>> + return -EINVAL;
>>> +
>>> + s->flags &= ~SLAB_FAILSLAB;
>>> + if (buf[0] == '1')
>>> + s->flags |= SLAB_FAILSLAB;
>>
>> Could we at least use a temporary variable to set up the final value and
>> then do a WRITE_ONCE() to s->flags, so the compiler is not allowed to do
>> some funky stuff? Assuming this is really the only place where we modify
>> s->flags during runtime, so we can't miss other updates due to RMW.
>
> Since it is set or clear - instead of temporary variable and potentially two
> writes and RMW issues i would suggest this:
> + if (buf[0] == '1')
> + s->flags |= SLAB_FAILSLAB;
> + else
> + s->flags &= ~SLAB_FAILSLAB;
This way also has RMW issues, and also the compiler is allowed to
temporarily modify s->flags any way it likes; with WRITE_ONCE() it can't.
> If at some point more places need to modify the flags at runtime they can
> switch to atomic bit ops.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: Make failslab writable again
2022-09-20 9:29 ` Vlastimil Babka
@ 2022-09-20 10:21 ` Alexander Atanasov
2022-09-20 10:31 ` Vlastimil Babka
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Atanasov @ 2022-09-20 10:21 UTC (permalink / raw)
To: Vlastimil Babka, Jonathan Corbet, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
Roman Gushchin, Hyeonggon Yoo
Cc: kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta,
linux-doc, linux-kernel, linux-mm
On 20.09.22 12:29, Vlastimil Babka wrote:
> On 9/20/22 11:17, Alexander Atanasov wrote:
>> Hello,
>>
>> On 20.09.22 11:42, Vlastimil Babka wrote:
>>>> +static ssize_t failslab_store(struct kmem_cache *s, const char *buf,
>>>> + size_t length)
>>>> +{
>>>> + if (s->refcount > 1)
>>>> + return -EINVAL;
>>>> +
>>>> + s->flags &= ~SLAB_FAILSLAB;
>>>> + if (buf[0] == '1')
>>>> + s->flags |= SLAB_FAILSLAB;
>>>
>>> Could we at least use a temporary variable to set up the final value and
>>> then do a WRITE_ONCE() to s->flags, so the compiler is not allowed to do
>>> some funky stuff? Assuming this is really the only place where we modify
>>> s->flags during runtime, so we can't miss other updates due to RMW.
>>
>> Since it is set or clear - instead of temporary variable and potentially two
>> writes and RMW issues i would suggest this:
>> + if (buf[0] == '1')
>> + s->flags |= SLAB_FAILSLAB;
>> + else
>> + s->flags &= ~SLAB_FAILSLAB;
>
> This way also has RMW issues, and also the compiler is allowed to
> temporarily modify s->flags any way it likes; with WRITE_ONCE() it can't.
Okay, so the safest way is this?
if (buf[0] == '1')
WRITE_ONCE(s->flags, READ_ONCE(s->flags) | SLAB_FAILSLAB);
else
WRITE_ONCE(s->flags, READ_ONCE(s->flags) & ~SLAB_FAILSLAB);
It got me thinking how many places would break if the compiler
starts to temporariliy modify the flags - i hope it never does.
--
Regards,
Alexander Atanasov
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: Make failslab writable again
2022-09-20 10:21 ` Alexander Atanasov
@ 2022-09-20 10:31 ` Vlastimil Babka
0 siblings, 0 replies; 6+ messages in thread
From: Vlastimil Babka @ 2022-09-20 10:31 UTC (permalink / raw)
To: Alexander Atanasov, Jonathan Corbet, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
Roman Gushchin, Hyeonggon Yoo
Cc: kernel, Kees Cook, Roman Gushchin, Jann Horn, Vijayanand Jitta,
linux-doc, linux-kernel, linux-mm
On 9/20/22 12:21, Alexander Atanasov wrote:
> On 20.09.22 12:29, Vlastimil Babka wrote:
>> On 9/20/22 11:17, Alexander Atanasov wrote:
>>> Hello,
>>>
>>> On 20.09.22 11:42, Vlastimil Babka wrote:
>>>>> +static ssize_t failslab_store(struct kmem_cache *s, const char *buf,
>>>>> + size_t length)
>>>>> +{
>>>>> + if (s->refcount > 1)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + s->flags &= ~SLAB_FAILSLAB;
>>>>> + if (buf[0] == '1')
>>>>> + s->flags |= SLAB_FAILSLAB;
>>>>
>>>> Could we at least use a temporary variable to set up the final value and
>>>> then do a WRITE_ONCE() to s->flags, so the compiler is not allowed to do
>>>> some funky stuff? Assuming this is really the only place where we modify
>>>> s->flags during runtime, so we can't miss other updates due to RMW.
>>>
>>> Since it is set or clear - instead of temporary variable and potentially two
>>> writes and RMW issues i would suggest this:
>>> + if (buf[0] == '1')
>>> + s->flags |= SLAB_FAILSLAB;
>>> + else
>>> + s->flags &= ~SLAB_FAILSLAB;
>>
>> This way also has RMW issues, and also the compiler is allowed to
>> temporarily modify s->flags any way it likes; with WRITE_ONCE() it can't.
>
> Okay, so the safest way is this?
>
> if (buf[0] == '1')
> WRITE_ONCE(s->flags, READ_ONCE(s->flags) | SLAB_FAILSLAB);
> else
> WRITE_ONCE(s->flags, READ_ONCE(s->flags) & ~SLAB_FAILSLAB);
Yeah, that would work. Given we are the only writer, we shouldn't even need
a READ_ONCE.
> It got me thinking how many places would break if the compiler
> starts to temporariliy modify the flags - i hope it never does.
That's likely true as well. But the macros have been introduced for this
purpose AFAIK.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-20 10:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-20 8:20 [PATCH] mm: Make failslab writable again Alexander Atanasov
2022-09-20 8:42 ` Vlastimil Babka
2022-09-20 9:17 ` Alexander Atanasov
2022-09-20 9:29 ` Vlastimil Babka
2022-09-20 10:21 ` Alexander Atanasov
2022-09-20 10:31 ` Vlastimil Babka
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.