linux-kernel - Re: [ceph] what's going on with d_rehash() in 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: <B17B70FF-4B76-4593-9D96-F0298F83AEB3@redhat.com>
Date:	Wed, 2 Mar 2016 11:00:01 +0800
From:	"Yan, Zheng" <zyan@...hat.com>
To:	Sage Weil <sweil@...hat.com>
Cc:	Al Viro <viro@...IV.linux.org.uk>, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, ceph-devel@...r.kernel.org
Subject: Re: [ceph] what's going on with d_rehash() in splice_dentry()?


> On Mar 1, 2016, at 22:50, Sage Weil <sweil@...hat.com> wrote:
> 
> Hi Al,
> 
> On Fri, 26 Feb 2016, Al Viro wrote:
>> You have, modulo printks and BUG_ON(),
>> {
>>        struct dentry *realdn;
>>        /* dn must be unhashed */
>>        if (!d_unhashed(dn))
>>                d_drop(dn);
>>        realdn = d_splice_alias(in, dn);
>>        if (IS_ERR(realdn)) {
>>                if (prehash)
>>                        *prehash = false; /* don't rehash on error */
>>                dn = realdn; /* note realdn contains the error */
>>                goto out;
>>        } else if (realdn) {
>>                dput(dn);
>>                dn = realdn;
>>        }
>>        if ((!prehash || *prehash) && d_unhashed(dn))
>>                d_rehash(dn);
>> 
>> When d_splice_alias() returns NULL it has hashed the dentry you'd given it;
>> when it returns a different dentry, that dentry is also returned hashed.
>> IOW, d_rehash(dn) in there should never be called.
>> 
>> If you have a case when it _is_ called, you've found a bug somewhere and
>> I'd like to see details.  AFAICS, the whole prehash thing appears to be
>> pointless - even the place where we modify *prehash, since in that case
>> we return ERR_PTR() and the only caller passing non-NULL prehash (&have_lease)
>> buggers off on such return value past all code that would look at have_lease
>> value.
> 
> Right.
> 
>> One possible reading is that you want to prevent hashing in !have_lease
>> case of
>>                        dn = splice_dentry(dn, in, &have_lease);
>> If that's the case, you might have a problem, since it will be hashed no
>> matter what...
> 
> In this case it doesn't actually matter if it is hashed or not, since 
> we will look at the lease state on the dentry before trusting it...
> 
> This code dates back to when Ceph was originally upstreamed, so the 
> history is murky, but I expect at that point I wanted to avoid hashing in 
> the no-lease case.  But I don't think it matters.  We should just remove 
> the prehash argument from splice_dentry entirely.
> 
> Zheng, does that sound right?

Yes. I think we can remove the d_rehash(dn) call and rehash parameter.

Regards
Yan, Zheng