linux-kernel - Re: [PATCH 03/12] staging: lustre: llite: fix various issues with ll_splice

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <244467F8-2775-4014-90FF-9CBC74F76B38@intel.com>
Date:   Wed, 19 Jul 2017 02:18:39 -0400
From:   Oleg Drokin <oleg.drokin@...el.com>
To:     NeilBrown <neilb@...e.com>
Cc:     Al Viro <viro@...IV.linux.org.uk>,
        Greg Kroah-Hartman <greg@...ah.com>,
        Andreas Dilger <andreas.dilger@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Lustre Development List <lustre-devel@...ts.lustre.org>
Subject: Re: [PATCH 03/12] staging: lustre: llite: fix various issues with ll_splice_alias.


On Jul 19, 2017, at 12:33 AM, NeilBrown wrote:

> On Tue, Jul 18 2017, Oleg Drokin wrote:
> 
>> Unfortunately this patch causes insta-crash on first stat call after mount.
>> Sorry, I cannot dig into this deeper right this moment, but I will a bit later.
> 
> V.strange.  The crash suggests that the lock, and hence the inode, is
> not initialized.  I cannot see how that might happen.
> though...
> 
>>> +		spin_lock(&lli->lli_lock);
>>> +		new = ll_find_invalid_alias(inode, de);
>>> +		if (!new)
>>> +			d_add(de, inode);
>>> +		spin_lock(&lli->lli_lock);
> 
> Had it not crashed, it would have deadlocked.  That second spin_lock()
> should be spin_unlock() :-( I don't *think* that would have caused this crash…

No, that's not it.
-	d_add(de, inode);
-	CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n",
-	       de, d_inode(de), d_count(de), de->d_flags);
+	de = d_splice_alias(inode, de);
+	if (!IS_ERR(de))
+		CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n",
+		       de, d_inode(de), d_count(de), de->d_flags);
	return de;

This is likely it.
d_splice_alias would return NULL if there's no alias, after d_add of the de.
But ll_splice_alias callers expect either an ERR_PTR on error or dentry to use
otherwise.
So in ll_lookup_it_finish() we have:
        alias = ll_splice_alias(inode, *de);
        if (IS_ERR(alias)) {
                rc = PTR_ERR(alias);
                goto out;
        }
        *de = alias;
…
                if (md_revalidate_lock(ll_i2mdexp(parent), &parent_it, &fid,
                                       NULL)) {
===>>> whoops!          d_lustre_revalidate(*de);
                        ll_intent_release(&parent_it);
                }

>> I am adding Al that we discussed this code at some length and he found no problems
>> here, so I am a bit surprised by your findings.
> I'd be very happy to read Al's thoughts.

Some of the discussions were in lkml under subject of "More parallel
atomic_open/d_splice_alias fun with NFS and possibly more FSes"
in July 2016, in between nfs stuff.
There was more but I cannot readily find it.

>> Also the reason we reinvent the d_splice_alias is because we need to
>> splice not just directories, but also regular files.
> 
> I see that.  A key simplification I bring is that directories and
> non-directories can be handled separately.  d_splice_alias() does
> all we need for directories, and nothing useful for non-dirs.

I see.
I still need to  think some more about this whole thing.

Please see commit 99f1c013194e64d4b67d5d318148303b0e1585e1 for double d_add
of the same dentry fix.6