Re: [PATCH ] drivers/md: use proper rcu accessor

Re: [PATCH ] drivers/md: use proper rcu accessor

[Mike Snitzer]

On Sun, Nov 23, 2014 at 11:40 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> rcu_dereference() should be used in sections protected by rcu_read_lock.
>
> For writers, holding some kind of mutex or lock,
> rcu_dereference_protected() is the way to go, adding explicit lockdep
> bits.
>
> In __unbind(), although there is no mutex or lock held, we are about
> to free the mapped device, so can use the constant '1' instead of a
> lockdep_is_held()

That isn't true.  dm_hash_remove_all() -- which calls dm_destroy --
holds _hash_lock.  Why leave __unbind() brittle in the face of future
DM locking changes?

> Reported-by: Kirill A. Shutemov <kirill@shutemov.name>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 33423974bfc1 ("dm: Use rcu_dereference() for accessing rcu pointer")
> Cc: Pranith Kumar <bobby.prani@gmail.com>

Hi Eric,

I'll pick this up once I get clarification for why your __unbind
change is safe.. but it really would've helped if you cc'd
dm-devel@redhat.com or myself directly (not a single person that you
cc'd actively maintains DM).

Hopefully these DM rcu "fixes" are finished after this.

Thanks,
Mike

Re: [PATCH ] drivers/md: use proper rcu accessor

[Eric Dumazet]

On Sun, 2014-11-23 at 11:53 -0500, Mike Snitzer wrote:
> On Sun, Nov 23, 2014 at 11:40 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > rcu_dereference() should be used in sections protected by rcu_read_lock.
> >
> > For writers, holding some kind of mutex or lock,
> > rcu_dereference_protected() is the way to go, adding explicit lockdep
> > bits.
> >
> > In __unbind(), although there is no mutex or lock held, we are about
> > to free the mapped device, so can use the constant '1' instead of a
> > lockdep_is_held()
> 
> That isn't true.  dm_hash_remove_all() -- which calls dm_destroy --
> holds _hash_lock.  Why leave __unbind() brittle in the face of future
> DM locking changes?
> 

Well, tell me. Before the 33423974bfc1 patch there was no protection.

If really you are about to delete an object, you have to be sure no one
is going to use it.

rcu_dereference_protected(X, 1) is how we express this thing, there is
nothing wrong here.

Fact that you hold a lock at this point is irrelevant and wont protect
the bug from happening. If you believe so, then you are wrong.


> > Reported-by: Kirill A. Shutemov <kirill@shutemov.name>
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Fixes: 33423974bfc1 ("dm: Use rcu_dereference() for accessing rcu pointer")
> > Cc: Pranith Kumar <bobby.prani@gmail.com>
> 
> Hi Eric,
> 
> I'll pick this up once I get clarification for why your __unbind
> change is safe.. but it really would've helped if you cc'd
> dm-devel@redhat.com or myself directly (not a single person that you
> cc'd actively maintains DM).
> 

Hmm, my mailer complained because the mail had too many recipients
already. I did a 'reply' on the original thread.

> Hopefully these DM rcu "fixes" are finished after this.

You added a Signed-off-by on 33423974bfc1, not me.

Kirill gave the report 2 days ago and so far nobody fixed it.

I will send a v2 because other rcu_dereference() need to be changed as
well.

[PATCH v2] drivers/md: use proper rcu accessor

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

rcu_dereference() should be used in sections protected by rcu_read_lock.

For writers, holding some kind of mutex or lock,
rcu_dereference_protected() is the way to go, adding explicit lockdep
bits.

In __unbind(), we are the last user of this mapped device, so can use
the constant '1' instead of a lockdep_is_held(), not consistent with
other uses of rcu_dereference_protected() which use md->suspend_lock
mutex.

Reported-by: Kirill A. Shutemov <kirill@shutemov.name>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: 33423974bfc1 ("dm: Use rcu_dereference() for accessing rcu pointer")
Cc: Pranith Kumar <bobby.prani@gmail.com>
Cc: Mike Snitzer <snitzer@redhat.com>
---

v2: changed all buggy rcu_dereference()
    BTW, having a mutex named suspend_lock is ugly, IMO.

 drivers/md/dm.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index a0ece87ad426..5919d933bce9 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2335,7 +2335,8 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
 
 	merge_is_optional = dm_table_merge_is_optional(t);
 
-	old_map = rcu_dereference(md->map);
+	old_map = rcu_dereference_protected(md->map,
+					    lockdep_is_held(&md->suspend_lock));
 	rcu_assign_pointer(md->map, t);
 	md->immutable_target_type = dm_table_get_immutable_target_type(t);
 
@@ -2355,7 +2356,7 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
  */
 static struct dm_table *__unbind(struct mapped_device *md)
 {
-	struct dm_table *map = rcu_dereference(md->map);
+	struct dm_table *map = rcu_dereference_protected(md->map, 1);
 
 	if (!map)
 		return NULL;
@@ -2850,7 +2851,8 @@ retry:
 		goto retry;
 	}
 
-	map = rcu_dereference(md->map);
+	map = rcu_dereference_protected(md->map,
+					lockdep_is_held(&md->suspend_lock));
 
 	r = __dm_suspend(md, map, suspend_flags, TASK_INTERRUPTIBLE);
 	if (r)
@@ -2908,7 +2910,8 @@ retry:
 		goto retry;
 	}
 
-	map = rcu_dereference(md->map);
+	map = rcu_dereference_protected(md->map,
+					lockdep_is_held(&md->suspend_lock));
 	if (!map || !dm_table_get_size(map))
 		goto out;
 
@@ -2943,7 +2946,8 @@ static void __dm_internal_suspend(struct mapped_device *md, unsigned suspend_fla
 		return; /* nest suspend */
 	}
 
-	map = rcu_dereference(md->map);
+	map = rcu_dereference_protected(md->map,
+					lockdep_is_held(&md->suspend_lock));
 
 	/*
 	 * Using TASK_UNINTERRUPTIBLE because only NOFLUSH internal suspend is

Re: [PATCH ] drivers/md: use proper rcu accessor

[Mike Snitzer]

On Sun, Nov 23 2014 at 12:31pm -0500,
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Sun, 2014-11-23 at 11:53 -0500, Mike Snitzer wrote:
> > On Sun, Nov 23, 2014 at 11:40 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > >
> > > rcu_dereference() should be used in sections protected by rcu_read_lock.
> > >
> > > For writers, holding some kind of mutex or lock,
> > > rcu_dereference_protected() is the way to go, adding explicit lockdep
> > > bits.
> > >
> > > In __unbind(), although there is no mutex or lock held, we are about
> > > to free the mapped device, so can use the constant '1' instead of a
> > > lockdep_is_held()
> > 
> > That isn't true.  dm_hash_remove_all() -- which calls dm_destroy --
> > holds _hash_lock.  Why leave __unbind() brittle in the face of future
> > DM locking changes?
> > 
> 
> Well, tell me. Before the 33423974bfc1 patch there was no protection.

Wasn't protected by _hash_lock or wasn't protected by use of rcu_deference()?
 
> If really you are about to delete an object, you have to be sure no one
> is going to use it.
> 
> rcu_dereference_protected(X, 1) is how we express this thing, there is
> nothing wrong here.
> 
> Fact that you hold a lock at this point is irrelevant and wont protect
> the bug from happening. If you believe so, then you are wrong.

My asking a question about the validity of your assertion given that
assertion wasn't 100% correct is perfectly fair no?  I just want to make
sure the change is accurately described.  Asking that question also
doesn't imply I felt you don't know what you're doing.  Fact is I really
trust you to be a _very_ capable developer.

But exactly which "bug" are we talking about?  A theoretical bug or a
reported bug that caused DM to fail?  So far all of these supposed rcu
deference fixes have _never_ substantiated with an actual bug (Other
than a splat from autochecking performed by rcu_dereference_check).

In fact the very first "fix" from 33423974bfc1 _seems_ to just be the
by-product from rcu janitor efforts.  I'm happy others are looking after
rcu consumers but pretty sure all of this churn is what is causing these
"bugs".

But maybe I'm mistaken and all these changes shoul dbe cc'ing
stable@vger.kernel.org?

> > > Reported-by: Kirill A. Shutemov <kirill@shutemov.name>
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > Fixes: 33423974bfc1 ("dm: Use rcu_dereference() for accessing rcu pointer")
> > > Cc: Pranith Kumar <bobby.prani@gmail.com>
> > 
> > Hi Eric,
> > 
> > I'll pick this up once I get clarification for why your __unbind
> > change is safe.. but it really would've helped if you cc'd
> > dm-devel@redhat.com or myself directly (not a single person that you
> > cc'd actively maintains DM).
> > 
> 
> Hmm, my mailer complained because the mail had too many recipients
> already. I did a 'reply' on the original thread.

It's OK, I missed the report from 2 days ago too (wasn't cc'd etc).

> > Hopefully these DM rcu "fixes" are finished after this.
> 
> You added a Signed-off-by on 33423974bfc1, not me.

Right, I don't pretend to keep my finger on the pulse of rcu API as much
as I probably should.  But I'm pretty sure Paul McKenney does and he
also provided his Signed-off-by -- and I really trust Paul on rcu stuff.

> Kirill gave the report 2 days ago and so far nobody fixed it.
> 
> I will send a v2 because other rcu_dereference() need to be changed as
> well.

I'm still wondering if they _need_ to be changed -- we aren't already
protected in these paths due to DM's existing locking (via suspend_lock
or _hash_lock, etc)?  But I'll circle back to try to properly understand
the need shortly.

Anyway, all being said: I appreciate your help here.  Not liking that we
got to baiting one another; I'll refrain from doing so in the future.

Re: [PATCH v2] drivers/md: use proper rcu accessor

[Mike Snitzer]

On Sun, Nov 23 2014 at 12:34pm -0500,
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> From: Eric Dumazet <edumazet@google.com>
> 
> rcu_dereference() should be used in sections protected by rcu_read_lock.
> 
> For writers, holding some kind of mutex or lock,
> rcu_dereference_protected() is the way to go, adding explicit lockdep
> bits.
> 
> In __unbind(), we are the last user of this mapped device, so can use
> the constant '1' instead of a lockdep_is_held(), not consistent with
> other uses of rcu_dereference_protected() which use md->suspend_lock
> mutex.
> 
> Reported-by: Kirill A. Shutemov <kirill@shutemov.name>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 33423974bfc1 ("dm: Use rcu_dereference() for accessing rcu pointer")
> Cc: Pranith Kumar <bobby.prani@gmail.com>
> Cc: Mike Snitzer <snitzer@redhat.com>

Thanks, I've staged this for 3.19:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-for-3.19&id=a12f5d48bdfeb5fe10157ac01c3de29269f457c6