Re: d_lookup: Unable to handle kernel paging request

Vicente Bergas
2019-06-19T16:51:57Z

On Wednesday, June 19, 2019 6:28:02 PM CEST, Al Viro wrote:
> [arm64 maintainers Cc'd; I'm not adding a Cc to moderated list,
> sorry]
>
> On Wed, Jun 19, 2019 at 02:42:16PM +0200, Vicente Bergas wrote:
>
>> Hi Al,
>> i have been running the distro-provided kernel the last few weeks
>> and had no issues at all.
>> https://archlinuxarm.org/packages/aarch64/linux-aarch64
>> It is from the v5.1 branch and is compiled with gcc 8.3.
>>
>> IIRC, i also tested
>> https://archlinuxarm.org/packages/aarch64/linux-aarch64-rc
>> v5.2-rc1 and v5.2-rc2 (which at that time where compiled with
>> gcc 8.2) with no issues.
>>
>> This week tested v5.2-rc4 and v5.2-rc5 from archlinuxarm but
>> there are regressions unrelated to d_lookup.
>>
>> At this point i was convinced it was a gcc 9.1 issue and had
>> nothing to do with the kernel, but anyways i gave your patch a try.
>> The tested kernel is v5.2-rc5-224-gbed3c0d84e7e and
>> it has been compiled with gcc 8.3.
>> The sentinel you put there has triggered!
>> So, it is not a gcc 9.1 issue.
>>
>> In any case, i have no idea if those addresses are arm64-specific
>> in any way.
>
> Cute... So *all* of those are in dentry_hashtable itself. IOW, we have
> these two values (1<<24 and (1<<24)|(0x88L<<40)) cropping up in
> dentry_hashtable[...].first on that config.
>
> That, at least, removes the possibility of corrupted forward pointer in
> the middle of a chain, with several pointers traversed before we run
> into something unmapped - the crap is in the very beginning.
>
> I don't get it. The only things modifying these pointers should be:
>
> static void ___d_drop(struct dentry *dentry)
> {
> struct hlist_bl_head *b;
> /*
> * Hashed dentries are normally on the dentry hashtable,
> * with the exception of those newly allocated by
> * d_obtain_root, which are always IS_ROOT:
> */
> if (unlikely(IS_ROOT(dentry)))
> b = &dentry->d_sb->s_roots;
> else
> b = d_hash(dentry->d_name.hash);
>
> hlist_bl_lock(b);
> __hlist_bl_del(&dentry->d_hash);
> hlist_bl_unlock(b);
> }
>
> and
>
> static void __d_rehash(struct dentry *entry)
> {
> struct hlist_bl_head *b = d_hash(entry->d_name.hash);
>
> hlist_bl_lock(b);
> hlist_bl_add_head_rcu(&entry->d_hash, b);
> hlist_bl_unlock(b);
> }
>
> The latter sets that pointer to (unsigned long)&entry->d_hash |
> LIST_BL_LOCKMASK),
> having dereferenced entry->d_hash prior to that. It can't be
> the source of those
> values, or we would've oopsed right there.
>
> The former... __hlist_bl_del() does
> /* pprev may be `first`, so be careful not to lose the lock bit */
> WRITE_ONCE(*pprev,
> (struct hlist_bl_node *)
> ((unsigned long)next |
> ((unsigned long)*pprev & LIST_BL_LOCKMASK)));
> if (next)
> next->pprev = pprev;
> so to end up with that garbage in the list head we'd have to had next
> the same bogus pointer (modulo bit 0, possibly). And since it's non-NULL,
> we would've immediately oopsed on trying to set next->pprev.
>
> There shouldn't be any pointers to hashtable elements other
> than ->d_hash.pprev
> of various dentries. And ->d_hash is not a part of anon unions in struct
> dentry, so it can't be mistaken access through the aliasing member.
>
> Of course, there's always a possibility of something stomping
> on random places
> in memory and shitting those values all over, with the hashtable being the
> hottest place on the loads where it happens... Hell knows...
>
> What's your config, BTW? SMP and DEBUG_SPINLOCK, specifically...

Hi Al,
here it is:
https://paste.debian.net/1088517

Regards,
Vicenç.