RE: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t

[Reshetova, Elena]

> On Tue, 2017-03-14 at 12:29 +, Reshetova, Elena wrote:
> > > Elena Reshetova  writes:
> > >
> > > > refcount_t type and corresponding API should be
> > > > used instead of atomic_t when the variable is used as
> > > > a reference counter. This allows to avoid accidental
> > > > refcounter overflows that might lead to use-after-free
> > > > situations.
> > > >
> > > > Signed-off-by: Elena Reshetova 
> > > > Signed-off-by: Hans Liljestrand 
> > > > Signed-off-by: Kees Cook 
> > > > Signed-off-by: David Windsor 
> > > > ---
> > > >  drivers/md/md.c | 6 +++---
> > > >  drivers/md/md.h | 3 ++-
> > > >  2 files changed, 5 insertions(+), 4 deletions(-)
> > >
> > > When booting linux-next (specifically 5be4921c9958ec) I'm seeing
> > > the
> > > backtrace below. I suspect this patch is just exposing an existing
> > > issue?
> >
> > Yes, we have actually been following this issue in the another
> > thread.
> > It looks like the object is re-used somehow, but I can't quite
> > understand how just by reading the code.
> > This was what I put into the previous thread:
> >
> > "The log below indicates that you are using your refcounter in a bit
> > weird way in mddev_find().
> > However, I can't find the place (just by reading the code) where you
> > would increment refcounter from zero (vs. setting it to one).
> > It looks like you either iterate over existing nodes (and increment
> > their counters, which should be >= 1 at the time of increment) or
> > create a new node, but then mddev_init() sets the counter to 1. "
> >
> > If you can help to understand what is going on with the object
> > creation/destruction, would be appreciated!
> >
> > Also Shaohua Li stopped this patch coming from his tree since the
> > issue was caught at that time, so we are not going to merge this
> > until we figure it out.
> 
> Asking on the correct list (dm-devel) would have got you the easy
> answer:  The refcount behind mddev->active is a genuine atomic.  It has
> refcount properties but only if the array fails to initialise (in that
> case, final put kills it).  Once it's added to the system as a gendisk,
> it cannot be freed until md_free().  Thus its ->active count can go to
> zero (when it becomes inactive; usually because of an unmount). On a
> simple allocation regardless of outcome, the last executed statement in
> md_alloc is mddev_put(): that destroys the device if we didn't manage
> to create it or returns 0 and adds an inactive device to the system
> which the user can get with mddev_find().

Thank you James for explaining this! I guess in this case, the conversion 
doesn't make sense. 
And sorry about not asking in a correct place: we are handling many similar 
patches now and while I try to reach the right audience using get_maintainer 
script, it doesn't always succeeds. 

Best Regards,
Elena.

> 
> James
> 

Re: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t

[James Bottomley]

On Tue, 2017-03-14 at 12:29 +, Reshetova, Elena wrote:
> > Elena Reshetova  writes:
> > 
> > > refcount_t type and corresponding API should be
> > > used instead of atomic_t when the variable is used as
> > > a reference counter. This allows to avoid accidental
> > > refcounter overflows that might lead to use-after-free
> > > situations.
> > > 
> > > Signed-off-by: Elena Reshetova 
> > > Signed-off-by: Hans Liljestrand 
> > > Signed-off-by: Kees Cook 
> > > Signed-off-by: David Windsor 
> > > ---
> > >  drivers/md/md.c | 6 +++---
> > >  drivers/md/md.h | 3 ++-
> > >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > When booting linux-next (specifically 5be4921c9958ec) I'm seeing
> > the
> > backtrace below. I suspect this patch is just exposing an existing
> > issue?
> 
> Yes, we have actually been following this issue in the another
> thread. 
> It looks like the object is re-used somehow, but I can't quite
> understand how just by reading the code. 
> This was what I put into the previous thread:
> 
> "The log below indicates that you are using your refcounter in a bit
> weird way in mddev_find(). 
> However, I can't find the place (just by reading the code) where you
> would increment refcounter from zero (vs. setting it to one).
> It looks like you either iterate over existing nodes (and increment
> their counters, which should be >= 1 at the time of increment) or
> create a new node, but then mddev_init() sets the counter to 1. "
> 
> If you can help to understand what is going on with the object
> creation/destruction, would be appreciated!
> 
> Also Shaohua Li stopped this patch coming from his tree since the 
> issue was caught at that time, so we are not going to merge this 
> until we figure it out. 

Asking on the correct list (dm-devel) would have got you the easy
answer:  The refcount behind mddev->active is a genuine atomic.  It has
refcount properties but only if the array fails to initialise (in that
case, final put kills it).  Once it's added to the system as a gendisk,
it cannot be freed until md_free().  Thus its ->active count can go to
zero (when it becomes inactive; usually because of an unmount). On a
simple allocation regardless of outcome, the last executed statement in
md_alloc is mddev_put(): that destroys the device if we didn't manage
to create it or returns 0 and adds an inactive device to the system
which the user can get with mddev_find().

James

RE: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t

[Reshetova, Elena]

> Elena Reshetova  writes:
> 
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  drivers/md/md.c | 6 +++---
> >  drivers/md/md.h | 3 ++-
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> When booting linux-next (specifically 5be4921c9958ec) I'm seeing the
> backtrace below. I suspect this patch is just exposing an existing
> issue?

Yes, we have actually been following this issue in the another thread. 
It looks like the object is re-used somehow, but I can't quite understand how 
just by reading the code. 
This was what I put into the previous thread:

"The log below indicates that you are using your refcounter in a bit weird way 
in mddev_find(). 
However, I can't find the place (just by reading the code) where you would 
increment refcounter from zero (vs. setting it to one).
It looks like you either iterate over existing nodes (and increment their 
counters, which should be >= 1 at the time of increment) or create a new node, 
but then mddev_init() sets the counter to 1. "

If you can help to understand what is going on with the object 
creation/destruction, would be appreciated!

Also Shaohua Li stopped this patch coming from his tree since the issue was 
caught at that time, so we are not going to merge this until we figure it out. 

Best Regards,
Elena.

> 
> cheers
> 
> 
> [0.230738] md: Waiting for all devices to be available before autodetect
> [0.230742] md: If you don't use raid, use raid=noautodetect
> [0.230962] refcount_t: increment on 0; use-after-free.
> [0.230988] [ cut here ]
> [0.230996] WARNING: CPU: 0 PID: 1 at lib/refcount.c:114
> .refcount_inc+0x5c/0x70
> [0.231001] Modules linked in:
> [0.231006] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.11.0-rc1-gccN-next-
> 20170310-g5be4921 #1
> [0.231012] task: c0004940 task.stack: c0004944
> [0.231016] NIP: c05ac6bc LR: c05ac6b8 CTR:
> c0743390
> [0.231021] REGS: c00049443160 TRAP: 0700   Not tainted  (4.11.0-rc1-
> gccN-next-20170310-g5be4921)
> [0.231026] MSR: 80029032 
> [0.231033]   CR: 24024422  XER: 000c
> [0.231038] CFAR: c0a5356c SOFTE: 1
> [0.231038] GPR00: c05ac6b8 c000494433e0 c1079d00
> 002b
> [0.231038] GPR04:  00ef 
> c10418a0
> [0.231038] GPR08: 4af8 c0ecc9a8 c0ecc9a8
> 
> [0.231038] GPR12: 28024824 c6bb 
> c00049443a00
> [0.231038] GPR16:  c00049443a10 
> 
> [0.231038] GPR20:   c0f7dd20
> 
> [0.231038] GPR24: 014080c0 c12060b8 c1206080
> 0009
> [0.231038] GPR28: c0f7dde0 0090 
> c000461ae800
> [0.231100] NIP [c05ac6bc] .refcount_inc+0x5c/0x70
> [0.231104] LR [c05ac6b8] .refcount_inc+0x58/0x70
> [0.231108] Call Trace:
> [0.231112] [c000494433e0] [c05ac6b8] .refcount_inc+0x58/0x70
> (unreliable)
> [0.231120] [c00049443450] [c086c008]
> .mddev_find+0x1e8/0x430
> [0.231125] [c00049443530] [c0872b6c] .md_open+0x2c/0x140
> [0.231132] [c000494435c0] [c03962a4]
> .__blkdev_get+0xd4/0x520
> [0.231138] [c00049443690] [c0396cc0] .blkdev_get+0x1c0/0x4f0
> [0.231145] [c00049443790] [c0336d64]
> .do_dentry_open.isra.1+0x2a4/0x410
> [0.231152] [c00049443830] [c03523f4]
> .path_openat+0x624/0x1580
> [0.231157] [c00049443990] [c0354ce4]
> .do_filp_open+0x84/0x120
> [0.231163] [c00049443b10] [c0338d74]
> .do_sys_open+0x214/0x300
> [0.231170] [c00049443be0] [c0da69ac]
> .md_run_setup+0xa0/0xec
> [0.231176] [c00049443c60] [c0da4fbc]
> .prepare_namespace+0x60/0x240
> [0.231182] [c00049443ce0] [c0da47a8]
> .kernel_init_freeable+0x330/0x36c
> [0.231190] [c00049443db0] [c000dc44] .kernel_init+0x24/0x160
> [0.231197] [c00049443e30] [c000badc]
> .ret_from_kernel_thread+0x58/0x7c
> [0.231202] Instruction dump:
> [0.231206] 6000 3d22ffee 89296bfb 2f89 409effdc 3c62ffc6 3921
> 3d42ffee
> [0.231216] 38630928 992a6bfb 484a6e79 6000 <0fe0> 4bb8
> 6000 6000
> [

Re: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t

[Michael Ellerman]

Elena Reshetova  writes:

> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
>
> Signed-off-by: Elena Reshetova 
> Signed-off-by: Hans Liljestrand 
> Signed-off-by: Kees Cook 
> Signed-off-by: David Windsor 
> ---
>  drivers/md/md.c | 6 +++---
>  drivers/md/md.h | 3 ++-
>  2 files changed, 5 insertions(+), 4 deletions(-)

When booting linux-next (specifically 5be4921c9958ec) I'm seeing the
backtrace below. I suspect this patch is just exposing an existing
issue?

cheers


[0.230738] md: Waiting for all devices to be available before autodetect
[0.230742] md: If you don't use raid, use raid=noautodetect
[0.230962] refcount_t: increment on 0; use-after-free.
[0.230988] [ cut here ]
[0.230996] WARNING: CPU: 0 PID: 1 at lib/refcount.c:114 
.refcount_inc+0x5c/0x70
[0.231001] Modules linked in:
[0.231006] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
4.11.0-rc1-gccN-next-20170310-g5be4921 #1
[0.231012] task: c0004940 task.stack: c0004944
[0.231016] NIP: c05ac6bc LR: c05ac6b8 CTR: c0743390
[0.231021] REGS: c00049443160 TRAP: 0700   Not tainted  
(4.11.0-rc1-gccN-next-20170310-g5be4921)
[0.231026] MSR: 80029032 
[0.231033]   CR: 24024422  XER: 000c
[0.231038] CFAR: c0a5356c SOFTE: 1 
[0.231038] GPR00: c05ac6b8 c000494433e0 c1079d00 
002b 
[0.231038] GPR04:  00ef  
c10418a0 
[0.231038] GPR08: 4af8 c0ecc9a8 c0ecc9a8 
 
[0.231038] GPR12: 28024824 c6bb  
c00049443a00 
[0.231038] GPR16:  c00049443a10  
 
[0.231038] GPR20:   c0f7dd20 
 
[0.231038] GPR24: 014080c0 c12060b8 c1206080 
0009 
[0.231038] GPR28: c0f7dde0 0090  
c000461ae800 
[0.231100] NIP [c05ac6bc] .refcount_inc+0x5c/0x70
[0.231104] LR [c05ac6b8] .refcount_inc+0x58/0x70
[0.231108] Call Trace:
[0.231112] [c000494433e0] [c05ac6b8] .refcount_inc+0x58/0x70 
(unreliable)
[0.231120] [c00049443450] [c086c008] .mddev_find+0x1e8/0x430
[0.231125] [c00049443530] [c0872b6c] .md_open+0x2c/0x140
[0.231132] [c000494435c0] [c03962a4] .__blkdev_get+0xd4/0x520
[0.231138] [c00049443690] [c0396cc0] .blkdev_get+0x1c0/0x4f0
[0.231145] [c00049443790] [c0336d64] 
.do_dentry_open.isra.1+0x2a4/0x410
[0.231152] [c00049443830] [c03523f4] .path_openat+0x624/0x1580
[0.231157] [c00049443990] [c0354ce4] .do_filp_open+0x84/0x120
[0.231163] [c00049443b10] [c0338d74] .do_sys_open+0x214/0x300
[0.231170] [c00049443be0] [c0da69ac] .md_run_setup+0xa0/0xec
[0.231176] [c00049443c60] [c0da4fbc] 
.prepare_namespace+0x60/0x240
[0.231182] [c00049443ce0] [c0da47a8] 
.kernel_init_freeable+0x330/0x36c
[0.231190] [c00049443db0] [c000dc44] .kernel_init+0x24/0x160
[0.231197] [c00049443e30] [c000badc] 
.ret_from_kernel_thread+0x58/0x7c
[0.231202] Instruction dump:
[0.231206] 6000 3d22ffee 89296bfb 2f89 409effdc 3c62ffc6 3921 
3d42ffee 
[0.231216] 38630928 992a6bfb 484a6e79 6000 <0fe0> 4bb8 6000 
6000 
[0.231226] ---[ end trace 8c51f269ad91ffc2 ]---
[0.231233] md: Autodetecting RAID arrays.
[0.231236] md: autorun ...
[0.231239] md: ... autorun DONE.
[0.234188] EXT4-fs (sda4): mounting ext3 file system using the ext4 
subsystem
[0.250506] refcount_t: underflow; use-after-free.
[0.250531] [ cut here ]
[0.250537] WARNING: CPU: 0 PID: 3 at lib/refcount.c:207 
.refcount_dec_not_one+0x104/0x120
[0.250542] Modules linked in:
[0.250546] CPU: 0 PID: 3 Comm: kworker/0:0 Tainted: GW   
4.11.0-rc1-gccN-next-20170310-g5be4921 #1
[0.250553] Workqueue: events .delayed_fput
[0.250557] task: c00049404900 task.stack: c00049448000
[0.250562] NIP: c05ac964 LR: c05ac960 CTR: c0743390
[0.250567] REGS: c0004944b530 TRAP: 0700   Tainted: GW
(4.11.0-rc1-gccN-next-20170310-g5be4921)
[0.250572] MSR: 80029032 
[0.250578]   CR: 24002422  XER: 0007
[0.250584] CFAR: c0a5356c SOFTE: 1 
[0.250584] GPR00: c05ac960 

Re: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t

[gre...@linuxfoundation.org]

On Wed, Mar 08, 2017 at 09:42:09AM +, Reshetova, Elena wrote:
> > On Mon, Mar 06, 2017 at 04:20:55PM +0200, Elena Reshetova wrote:
> > > refcount_t type and corresponding API should be
> > > used instead of atomic_t when the variable is used as
> > > a reference counter. This allows to avoid accidental
> > > refcounter overflows that might lead to use-after-free
> > > situations.
> > 
> > Looks good. Let me know how do you want to route the patch to upstream.
> 
> Greg, you previously mentioned that driver's conversions can go via your 
> tree. Does this still apply?
> Or should I be asking maintainers to merge these patches via their trees? 

You should ask them to take them through their trees, if they have them.
I'll be glad to scoop up all of the remaining ones that get missed, or
for subsystems that do not have trees.

thanks,

greg k-h

RE: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t

[Reshetova, Elena]

> On Mon, Mar 06, 2017 at 04:20:55PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> 
> Looks good. Let me know how do you want to route the patch to upstream.

Greg, you previously mentioned that driver's conversions can go via your tree. 
Does this still apply?
Or should I be asking maintainers to merge these patches via their trees? 
I am not sure about the correct (and easier for everyone) way, please suggest.  

Best Regards,
Elena.
> 
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  drivers/md/md.c | 6 +++---
> >  drivers/md/md.h | 3 ++-
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 985374f..94c8ebf 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -449,7 +449,7 @@ EXPORT_SYMBOL(md_unplug);
> >
> >  static inline struct mddev *mddev_get(struct mddev *mddev)
> >  {
> > -   atomic_inc(>active);
> > +   refcount_inc(>active);
> > return mddev;
> >  }
> >
> > @@ -459,7 +459,7 @@ static void mddev_put(struct mddev *mddev)
> >  {
> > struct bio_set *bs = NULL;
> >
> > -   if (!atomic_dec_and_lock(>active, _mddevs_lock))
> > +   if (!refcount_dec_and_lock(>active, _mddevs_lock))
> > return;
> > if (!mddev->raid_disks && list_empty(>disks) &&
> > mddev->ctime == 0 && !mddev->hold_active) {
> > @@ -495,7 +495,7 @@ void mddev_init(struct mddev *mddev)
> > INIT_LIST_HEAD(>all_mddevs);
> > setup_timer(>safemode_timer, md_safemode_timeout,
> > (unsigned long) mddev);
> > -   atomic_set(>active, 1);
> > +   refcount_set(>active, 1);
> > atomic_set(>openers, 0);
> > atomic_set(>active_io, 0);
> > spin_lock_init(>lock);
> > diff --git a/drivers/md/md.h b/drivers/md/md.h
> > index b8859cb..4811663 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> > @@ -22,6 +22,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -360,7 +361,7 @@ struct mddev {
> >  */
> > struct mutexopen_mutex;
> > struct mutexreconfig_mutex;
> > -   atomic_tactive;
>   /* general refcount */
> > +   refcount_t  active;
>   /* general refcount */
> > atomic_topeners;/*
> number of active opens */
> >
> > int
>   changed;/* True if we might need to
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t

[Shaohua Li]

On Mon, Mar 06, 2017 at 04:20:55PM +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.

Looks good. Let me know how do you want to route the patch to upstream.
 
> Signed-off-by: Elena Reshetova 
> Signed-off-by: Hans Liljestrand 
> Signed-off-by: Kees Cook 
> Signed-off-by: David Windsor 
> ---
>  drivers/md/md.c | 6 +++---
>  drivers/md/md.h | 3 ++-
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 985374f..94c8ebf 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -449,7 +449,7 @@ EXPORT_SYMBOL(md_unplug);
>  
>  static inline struct mddev *mddev_get(struct mddev *mddev)
>  {
> - atomic_inc(>active);
> + refcount_inc(>active);
>   return mddev;
>  }
>  
> @@ -459,7 +459,7 @@ static void mddev_put(struct mddev *mddev)
>  {
>   struct bio_set *bs = NULL;
>  
> - if (!atomic_dec_and_lock(>active, _mddevs_lock))
> + if (!refcount_dec_and_lock(>active, _mddevs_lock))
>   return;
>   if (!mddev->raid_disks && list_empty(>disks) &&
>   mddev->ctime == 0 && !mddev->hold_active) {
> @@ -495,7 +495,7 @@ void mddev_init(struct mddev *mddev)
>   INIT_LIST_HEAD(>all_mddevs);
>   setup_timer(>safemode_timer, md_safemode_timeout,
>   (unsigned long) mddev);
> - atomic_set(>active, 1);
> + refcount_set(>active, 1);
>   atomic_set(>openers, 0);
>   atomic_set(>active_io, 0);
>   spin_lock_init(>lock);
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index b8859cb..4811663 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -360,7 +361,7 @@ struct mddev {
>*/
>   struct mutexopen_mutex;
>   struct mutexreconfig_mutex;
> - atomic_tactive; /* general refcount */
> + refcount_t  active; /* general refcount */
>   atomic_topeners;/* number of active 
> opens */
>  
>   int changed;/* True if we might 
> need to
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html