linux-kernel - Re: [GIT PULL] Ceph fixes for 5.1-rc7

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: <fd4327a68e79540e9faee40bd198dfee373e59fb.camel@kernel.org>
Date:   Fri, 26 Apr 2019 16:49:24 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Al Viro <viro@...iv.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Ilya Dryomov <idryomov@...il.com>, ceph-devel@...r.kernel.org,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] Ceph fixes for 5.1-rc7

On Fri, 2019-04-26 at 18:01 +0100, Al Viro wrote:
> On Fri, Apr 26, 2019 at 09:36:00AM -0700, Linus Torvalds wrote:
> > On Fri, Apr 26, 2019 at 9:25 AM Jeff Layton <jlayton@...nel.org> wrote:
> > > Is it really ok to union the count and rcu_head there?
> > 
> > It should be fine, because the rcu_count should only ever be used once
> > the count has gone to zero and the name cannot be found any more.
> > 
> > And while RCU path walking may find and use the *name* after the
> > dentry has been killed off (but not free'd yet), all the actual
> > external_name() accesses should be serialized by the dentry lock, so
> > there's no access to those fields once the dentry is dead.
> 
> It's not quite that; access to external_name contents is fine,
> ->d_lock or not.  __d_lookup_rcu() does read it under rcu_read_lock
> alone.
> 
> However:
> 	* we never free it without an RCU delay after the final
> drop of refcount.  RCU delay might happen on dentry->d_rcu (if
> it's dentry_free()) or on name->p.rcu (if it's release_dentry_name_snapshot()
> or d_move() dropping the final reference).
> 	* it's never observed in ->d_name after the refcount
> reaches zero.
> 	* no lockless access ever looks at the refcount.  It
> can look at ->name[], but that's it.
> 

Got it, thanks. Why use an atomic_t for the refcount if it's always
accessed under spinlock?

> What I don't understand is why would anyone want to mess with
> name snapshots for dentry_path() lookalikes...

Mostly because the place where the ceph code needs to use and free these
strings is rather far removed from where they are created. It simplifies
that part if we can access and free them all in the same way.

I was planning to just use name_snapshots universally for that purpose,
but they have a rather specific method of freeing things that is hard to
duplicate if you don't have a dentry to clone.

Probably that means I'm approaching this problem in the wrong way and
need to do it differently.
-- 
Jeff Layton <jlayton@...nel.org>