Messages in this thread | | | Date | Tue, 20 Sep 2022 12:17:23 +0300 | Subject | Re: [PATCH] mm: Make failslab writable again | From | Alexander Atanasov <> |
| |
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
| |