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 <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Roman Gushchin <[email protected]>
> Cc: Christoph Lameter <[email protected]>
> Cc: Jann Horn <[email protected]>
> Cc: Vijayanand Jitta <[email protected]>
> Cc: David Rientjes <[email protected]>
> Cc: Joonsoo Kim <[email protected]>
> Cc: Pekka Enberg <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
>
> Signed-off-by: Alexander Atanasov <[email protected]>
> ---
> 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
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
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.
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
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.