Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Thu, 05 Mar 2020 11:18:55 +0530
From:   vjitta@codeaurora.org
To:     Vlastimil Babka <vbabka@suse.cz>
Cc:     cl@linux.com, penberg@kernel.org, rientjes@google.com,
        iamjoonsoo.kim@lge.com, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        vinmenon@codeaurora.org, kernel-team@android.com
Subject: Re: [PATCH] mm: slub: reinitialize random sequence cache on slab
 object update
In-Reply-To: <d3acc069-a5c6-f40a-f95c-b546664bc4ee@suse.cz>
References: <1580379523-32272-1-git-send-email-vjitta@codeaurora.org>
 <1580383064-16536-1-git-send-email-vjitta@codeaurora.org>
 <d3acc069-a5c6-f40a-f95c-b546664bc4ee@suse.cz>
Message-ID: <da7f86610add1ea78234dfc88178472e@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-02-27 22:23, Vlastimil Babka wrote:
> On 1/30/20 12:17 PM, vjitta@codeaurora.org wrote:
>> From: Vijayanand Jitta <vjitta@codeaurora.org>
>> 
>> Random sequence cache is precomputed during slab object creation
>> based up on the object size and no of objects per slab. These could
>> be changed when flags like SLAB_STORE_USER, SLAB_POISON are updated
>> from sysfs. So when shuffle_freelist is called during slab_alloc it
>> uses updated object count to access the precomputed random sequence
>> cache. This could result in incorrect access of the random sequence
>> cache which could further result in slab corruption. Fix this by
>> reinitializing the random sequence cache up on slab object update.
>> 
>> A sample panic trace when write to slab_store_user was attempted.
> 
> A more complete oops report would have been better, e.g. if anyone was 
> googling
> it, to find this patch.
> 
> Also I was checking where else calculate_sizes() is called and found
> order_store(). So if somebody changes (especially increases) the order,
> shouldn't the reinitialization also be done?

Yes, reinitialization must be done here aswell , will update the patch.

> 
> This is even more nasty as it doesn't seem to require that no objects 
> exist.
> Also there is no synchronization against concurrent allocations/frees? 
> Gasp.

Since, random sequence cache is only used to update the freelist in 
shuffle_freelist
which is done only when a new slab is created incase if objects 
allocations are
done without a need of new slab creation they will use the existing 
freelist which
should be fine as object size doesn't change after order_store() and 
incase if a new
slab is created we will get the updated freelist. so in both cases i 
think it should
be fine.