Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6f5a8f71-3d82-97bf-90e1-0f33546bb59b@virtuozzo.com>
Date:   Tue, 20 Sep 2022 12:17:23 +0300
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.2.2
Subject: Re: [PATCH] mm: Make failslab writable again
To:     Vlastimil Babka <vbabka@suse.cz>, Jonathan Corbet <corbet@lwn.net>,
        Christoph Lameter <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Hyeonggon Yoo <42.hyeyoo@gmail.com>
Cc:     kernel@openvz.org, Kees Cook <keescook@chromium.org>,
        Roman Gushchin <guro@fb.com>, Jann Horn <jannh@google.com>,
        Vijayanand Jitta <vjitta@codeaurora.org>,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
References: <20220920082033.1727374-1-alexander.atanasov@virtuozzo.com>
 <21646f5c-39ee-a51c-f30c-272de85ee350@suse.cz>
From:   Alexander Atanasov <alexander.atanasov@virtuozzo.com>
In-Reply-To: <21646f5c-39ee-a51c-f30c-272de85ee350@suse.cz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VmZ2UFNOckViSEVxUHdaK05yZXNkNXZqMjhteFk1b2htQ1dlOUJmbjh4WDly?=
 =?utf-8?B?WEFScFA0S0Y0MkE1YXdpLzJGeTdmcng5eDBCWGRXNEdBUGZDMWh0NFRNMWtr?=
 =?utf-8?B?b2ZUN0hjQlp0N2pPL2cxMERkRnVjUzcrVmdjbXVjZVc4NVEzYno5RStjNUZW?=
 =?utf-8?B?bkM2TnZrdElrWmQ2RHlxUW0zUGFKYmZwaHRFckV5NEVVbG11bEo5NGovSDVr?=
 =?utf-8?B?R0dRRjZJbENpbE1PV1dFRG95b1hHQUpvbzdrY0JrdHBUcFlkUEhZS2JJaVFy?=
 =?utf-8?B?c2U3TVZQSVd1SnV5QldNcy9uZ3VYbFNNUUxGQjJyYWhxMDB4NGVsa2JxOUFI?=
 =?utf-8?B?Uk41UGVuTHNXUmcxdzcwZ1hYUFo0SGwwTGNUR0ZrTHhCV3psNWwvUXpuYzVE?=
 =?utf-8?B?a3M3TWp1eEI0dERUQ2JudStqUm9NWDdhWVEvVnB2WEhHRFpNOCt6b2o1YUV0?=
 =?utf-8?B?MThUbVU0dlUwRWd0eUlXS2dZUFlSZThyWGgyYWpCanpjMy9aajlsQ2NnQ1dW?=
 =?utf-8?B?d1YzMmJzTGpXK0ZXRW1Ea0l6cG5zV0FaZE5IUUd2Zk9qM3dkTmdaeWpLMVpI?=
 =?utf-8?B?ZWlya2xJeE4wTUZPZlRRV1hBd0xnWHQyUGxlK0EwQUtkczh1RXVvWFgvbGtB?=
 =?utf-8?B?aU80dHRZYTY0WVVpTENCNFJhUklVNkN3MnNyV20vUENVdWtWMktvVEgwWGFV?=
 =?utf-8?B?b1JhRm11RVNYS0VVWHRtbnlISnVveEZRejJSOWFjZVloOXZLMzdlSWdzZHlC?=
 =?utf-8?B?RmhQSnF3eDJTdjNLRVpDS2ZqdTdrVkUzWFhNNHNvVnpUNEJKVGY0TFlEQUpW?=
 =?utf-8?B?UjNCcGZYWC96eVlIdTJBVFFnSDFHd1FFKzVSTUQxU3IrVi9pSldEZ2toZE95?=
 =?utf-8?B?Q083RmdwNXp1aS9FMjc3bGtPbGFab3RYajFNUldDanhYOFAxQmlScUVXanlo?=
 =?utf-8?B?cHBVTmVtMEhub21DOVRaZ21ON0ZGSWJYUVFxNXVIR0FSbzlmenQ0S3cwQnUv?=
 =?utf-8?B?eDFkZmJWOGJ5bzlHdTRQYmd1WGd5S3hXcWJvd0hhN1ZXODBKNmZ5cm9FcnBG?=
 =?utf-8?B?T1hlV1ErUlN4V3VmK25LL2NVY3hxNlFIaUVMM2IwYVNUd3VSZENBb1JjVHpD?=
 =?utf-8?B?OE9JenNYYyttKzVReHJOalFWejA5aFUvYWtCMi92T1B0TnVRbE41SXZwMldL?=
 =?utf-8?B?ZzRSOEJ3bTAxblpsN0RvV00xcFpxcFpNRTA1VmF0T254eXpDemtDaHJ3SWtZ?=
 =?utf-8?B?UGJYa0d4dXdpQzB6bDVDdmN5WWx0VUN0ZHpjbCtTb2UyWDlhM1VGYVliN285?=
 =?utf-8?B?NDVOSnEwRHNYUmhNRnpPU3NCYndnT0Q5V2gwVE5sMDZkZUMzZEJvc2tnNmE1?=
 =?utf-8?B?eGhoWEJoRGgybUUvaHJrTmpjbTBERVhRSVQ4cXhvdEQyTFJyUXF2dGpOeHVy?=
 =?utf-8?B?eWpneHdrZE11SXUwK1phMzFrNHBrYW9ReW1ORldjbEZoSWdpRCtUQzR4aktt?=
 =?utf-8?B?dmpCTE52cklITXdpTzV3YUh5bytka0ltUE92SUR0eFZ0cVhCRHBJb3U1bkZK?=
 =?utf-8?B?UW5BOStUQVEzTWNwNnM3RDV3N2R1MTU1ajBOMDNpcDdjR2h5by9aTFB6OGVP?=
 =?utf-8?B?ci92MjJhQTJsNTlrZEcySWNuTFl5Qm9mZnNnUER1dENINWZGSTNkUWxXU3pK?=
 =?utf-8?B?RmNqcCt0T2ZVWDN5c1BkODlNU1ZGYktTY0YxVnd5ekFoTG5NVWNkRE5RSG5o?=
 =?utf-8?B?UStxb3Z6NHpiWWFBdE5tVzZ6bzJSR0E5Q05VM0Q3aWg1MkVrM1dpemMybndV?=
 =?utf-8?B?VHp3STIxQnA0cDFCdHRPem5NK0RRTjBhcHYwaXlpYlU0aWkwTTFiL0xJQ0wz?=
 =?utf-8?B?TGdUamp3UklheDRtL3A3dVNzTTJGNmNUbzk1L3NnM0tZL3lmVEVHVGQ5OEM0?=
 =?utf-8?B?U2xjVWJpN2x3OUpOaURodGNIN284eG9NQVJ2MjZJQUNpZUQ0QTlnbWZteVpa?=
 =?utf-8?B?K0NTRUYwOFJiRi9ta0ZIK2poTGtjT285WXMxRmhiTzBHUzkreDZwMG8vUURj?=
 =?utf-8?B?Y2UxUzhRcGpSUlZWbnJDcEVOaEVoT3lWSWhnWTF6UXpsTG1VUWdmMHhseldj?=
 =?utf-8?B?alhrVWU5YVJiQWE1L0JhNVBQMzFPWWJhY2M5SmtWMG04L1VqSmVoc3c0R3Ur?=
 =?utf-8?B?TkE9PQ==?=
X-OriginatorOrg: virtuozzo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2c739355-27d4-4125-f1b0-08da9ae8eea7
X-MS-Exchange-CrossTenant-AuthSource: VE1PR08MB4765.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Sep 2022 09:17:25.4826
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 123N7OllmqtbEK5YsgPKUGF/F6YfpaCLqZ+4aLnhtejOmfEZJvv5c2F1FAoMRTv1WnJFWzdcVZ8tHXwr3bmgA724mKnMmlZf3/YpArH9IcbZ4H5GydDpr5eakkRVFh57
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR08MB6550
X-Spam-Status: No, score=-3.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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