> On Tue, 2017-03-14 at 12:29 +0000, Reshetova, Elena wrote: > > > Elena Reshetova <elena.reshetova@xxxxxxxxx> 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 <elena.reshetova@xxxxxxxxx> > > > > Signed-off-by: Hans Liljestrand <ishkamiel@xxxxxxxxx> > > > > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> > > > > Signed-off-by: David Windsor <dwindsor@xxxxxxxxx> > > > > --- > > > > 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 >
