RE: [PATCH 0/6] v4 block refcount conversion patches

[Reshetova, Elena]

> Elena Reshetova  writes:
> > Elena Reshetova (6):
> >   block: convert bio.__bi_cnt from atomic_t to refcount_t
> >   block: convert blk_queue_tag.refcnt from atomic_t to refcount_t
> >   block: convert blkcg_gq.refcnt from atomic_t to refcount_t
> >   block: convert io_context.active_ref from atomic_t to refcount_t
> >   block: convert bsg_device.ref_count from atomic_t to refcount_t
> >   drivers, block: convert xen_blkif.refcnt from atomic_t to refcount_t
> 
> Hi Elena,
> 
> While the bsg ref_count is cheap, do you have any numbers how the other
> conversions compare in performance (throughput and latency) vs atomics?
Hi Johannes,

The performance would depend on which "breed" of refcount_t is used underneath. 
We currently have 3 versions:

- refcount_t defaults to atomic_t (no CONFIG_REFCOUNT_FULL enabled, no arch. 
support)
  Impact is zero in this case since it is just atomic functions are used. 
- refcount_t uses arch. specific implementation (arch. enables 
ARCH_HAS_REFCOUNT)
 Impact depends on arch. implementation. Currently only x86 provides one. 
- refcount_t uses "full" arch. independent implementation.

Here are cycle numbers for comparing these 3 (https://lwn.net/Articles/728626/):
Just copy pasting for convenience:

">These are the cycle counts comparing a loop of refcount_inc() from 1
>to INT_MAX and back down to 0 (via refcount_dec_and_test()), between
>unprotected refcount_t (atomic_t), fully protected REFCOUNT_FULL
>(refcount_t-full), and this overflow-protected refcount (refcount_t-fast):

>2147483646 refcount_inc()s and 2147483647 refcount_dec_and_test()s:
cycles  protections
>atomic_t  82249267387  none
>refcount_t-fast82211446892 overflow, untested dec-to-zero
>refcount_t-full   144814735193 overflow, untested dec-to-zero, inc-from-zero"

So, the middle option (called here refcount_t-fast) with arch. specific
implementation gives a negligible impact. The "full" one is more pricey, but it 
is
disabled by default anyway, so only people who want strict security enable it.  

Are these numbers convincing enough that we don't have to measure
the block devices? :)

Best Regards,
Elena.


> 
> It should be quite easy to measure against a null_blk device.
> 
> Thanks a lot,
>Johannes
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

RE: [PATCH 0/5] v3 block subsystem refcounter conversions

[Reshetova, Elena]

> On 06/28/2017 05:58 AM, Reshetova, Elena wrote:
> >
> >> Subject: Re: [PATCH 0/5] v3 block subsystem refcounter conversions
> >>
> >> On Tue, Jun 27, 2017 at 6:26 AM, Jens Axboe <ax...@kernel.dk> wrote:
> >>> On 06/27/2017 05:39 AM, Elena Reshetova wrote:
> >>>> Changes in v3:
> >>>> No changes in patches apart from trivial rebases, but now by
> >>>> default refcount_t = atomic_t and uses all atomic standard operations
> >>>> unless CONFIG_REFCOUNT_FULL is enabled. This is a compromize for the
> >>>> systems that are critical on performance and cannot accept even
> >>>> slight delay on the refcounter operations.
> >>>
> >>> Is that true in 4.12-rc, or is that true in a later release once
> >>> Linus has pulled those changes in? If the latter, please resend
> >>> this when those changes are in, thanks.
> >>
> >> It's in -next currently ("locking/refcount: Create unchecked atomic_t
> >> implementation")
> >
> > I would really like to start discussion on the these patches asap
> > since it normally takes some adjustments etc. before they can be
> > merged and we want many changes to go into next release round and not
> > to miss the merge window.
> 
> As far as I'm concerned, there's no need for a discussion on these. If
> the other patches go in to make it as light weight as what we currently
> have, then I'm fine with it. I can queue it up for post initial merge
> submission.

Ok, fair enough. Thank you very much!

Best Regards,
Elena.

> 
> --
> Jens Axboe

RE: [PATCH 0/5] v3 block subsystem refcounter conversions

[Reshetova, Elena]

> Subject: Re: [PATCH 0/5] v3 block subsystem refcounter conversions
> 
> On Tue, Jun 27, 2017 at 6:26 AM, Jens Axboe  wrote:
> > On 06/27/2017 05:39 AM, Elena Reshetova wrote:
> >> Changes in v3:
> >> No changes in patches apart from trivial rebases, but now by
> >> default refcount_t = atomic_t and uses all atomic standard operations
> >> unless CONFIG_REFCOUNT_FULL is enabled. This is a compromize for the
> >> systems that are critical on performance and cannot accept even
> >> slight delay on the refcounter operations.
> >
> > Is that true in 4.12-rc, or is that true in a later release once
> > Linus has pulled those changes in? If the latter, please resend
> > this when those changes are in, thanks.
> 
> It's in -next currently ("locking/refcount: Create unchecked atomic_t
> implementation")

I would really like to start discussion on the these patches asap since it 
normally takes
 some adjustments etc. before they can be merged and we want many changes to go 
into
next release round and not to miss the merge window. 

Best Regards,
Elena.

> 
> -Kees
> 
> --
> Kees Cook
> Pixel Security

RE: [PATCH 0/5] v2: block subsystem refcounter conversions

[Reshetova, Elena]

> Hi Elena,
> 
> On Thu, Apr 20, 2017 at 04:10:16PM +, Reshetova, Elena wrote:
> >
> > > All the objections from DaveM on the amount of cycles spent on the
> > > new refcount_t apply to the block layer fast path operations as well.
> >
> > Ok, could you please indicate the correct way to measure the impact for the
> block layer?
> > We can do the measurements.
> >
> > Best Regards,
> > Elena.
> >
> > >
> > > Please don't send any more conversions until those have been resolved.
> 
> Like I suggested months ago, how about doing an efficient implementation of
> refcount_t which doesn't use the bloated cmpxchg loop?  Then there would be
> no
> need for endless performance arguments.  In fact, in PaX there are already
> example implementations for several architectures.  It's unfortunate that
> they're still being ignored for some reason.

Providing efficient arch. specific implementation for refcount_t is likely the 
next step
to make performance-sensitive places happy. However, in order to do it, we will 
need
to measure for each subsystem a) current atomic_t usage - base measurement 
b) pure refcount_t usage impact
c) arch. specific refcount_t impact 

Otherwise I have a chicken and egg problem here: people want better performance
and want to see arch. specific code for this, but in order to convince 
maintainer
in need of arch. specific code, I need to show that we do have indeed a 
performance issue. 

So, that's why I was endlessly asking for each subsystem that said that pointed 
out 
performance reasons to give hints on what should be measured. 
This way we can come back with real numbers (including for arch. specific 
implement) and
then decide what makes the most sense. 

> 
> At the very least, what is there now could probably be made about twice as 
> fast
> by removing the checks that don't actually help mitigate refcount overflow 
> bugs,
> specifically all the checks in refcount_dec(), and the part of refcount_inc()
> where it doesn't allow incrementing a 0 refcount.  Hint: if a refcount is 0, 
> the
> object has already been freed, so the attacker will have had the opportunity 
> to
> allocate it with contents they control already.

refcount_dec() is used very little through the code actually, it is more like 
an exception
case since in order to use it one must really be sure that refcounter doesn't 
drop to zero.
Removing the warn around it wouldn't likely affect much overall and thus it is 
better to
stay to discourage people of API itself :)

refcount_inc() is of course a different story, it is extensively used. I guess 
the perf issue
on checking increment from zero might only come from WARNs being printed,
but not really from an additional check here for zero since it is trivial and 
part of
the needed loop anyway. So, I think only removing the
WARNs might have any visible impact, but even this is probably not really that 
big. 

So, I think these changes won't really help adoption of interface if arguments 
against
is performance. If we do have a performance issue, I think arch. specific 
implementation
is likely the only way to considerably speed it up. 

> 
> Of course, having extra checks behind a debug option is fine.  But they should
> not be part of the base feature; the base feature should just be mitigation of
> reference count *overflows*.  It would be nice to do more, of course; but when
> the extra stuff prevents people from using refcount_t for performance reasons,
> it defeats the point of the feature in the first place.

Sure, but as I said above, I think the smaller tricks and fixes won't be 
convincing enough,
so their value is questionable. 

> 
> I strongly encourage anyone who has been involved in refcount_t to experiment
> with removing a reference count decrement somewhere in their kernel, then
> trying
> to exploit it to gain code execution.  If you don't know what you're trying to
> protect against, you will not know which defences work and which don't.

Well, we had successful CVEs and even exploits on this in the past. 
@Kees, do you store a list of them in the project?

What do you actually want to verify? The fact that if you manage to create 
use-after-free
situation, you are able to take advantage of it? 

Best Regards,
Elena.

> 
> - Eric

RE: [PATCH 0/5] v2: block subsystem refcounter conversions

[Reshetova, Elena]

> All the objections from DaveM on the amount of cycles spent on the
> new refcount_t apply to the block layer fast path operations as well.

Ok, could you please indicate the correct way to measure the impact for the 
block layer? 
We can do the measurements. 

Best Regards,
Elena.

> 
> Please don't send any more conversions until those have been resolved.

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 +0000, Reshetova, Elena wrote:
> > > Elena Reshetova <elena.reshet...@intel.com> 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.reshet...@intel.com>
> > > > Signed-off-by: Hans Liljestrand <ishkam...@gmail.com>
> > > > Signed-off-by: Kees Cook <keesc...@chromium.org>
> > > > Signed-off-by: David Windsor <dwind...@gmail.com>
> > > > ---
> > > >  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

[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 22/29] drivers, scsi: convert iscsi_task.refcount from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/09/2017 08:18 AM, Reshetova, Elena wrote:
> >> On Mon, Mar 06, 2017 at 04:21:09PM +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.
> >>>
> >>> Signed-off-by: Elena Reshetova <elena.reshet...@intel.com>
> >>> Signed-off-by: Hans Liljestrand <ishkam...@gmail.com>
> >>> Signed-off-by: Kees Cook <keesc...@chromium.org>
> >>> Signed-off-by: David Windsor <dwind...@gmail.com>
> >>
> >> This looks OK to me.
> >>
> >> Acked-by: Chris Leech <cle...@redhat.com>
> >
> > Thank you for review! Do you have a tree that can take this change?
> 
> Hi Elena,
> 
> iscsi like fcoe should go via the SCSI tree.

Thanks Johannes! Should I resend with "Acked-by" added in order for it to be 
picked up? 

Best Regards,
Elena.


> 
> Byte,
>   Johannes
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

RE: [Xen-devel] [PATCH 29/29] drivers, xen: convert grant_map.users from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/08/2017 08:49 AM, Reshetova, Elena wrote:
> >> On 03/06/2017 09:21 AM, 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.
> >>>
> >>> Signed-off-by: Elena Reshetova <elena.reshet...@intel.com>
> >>> Signed-off-by: Hans Liljestrand <ishkam...@gmail.com>
> >>> Signed-off-by: Kees Cook <keesc...@chromium.org>
> >>> Signed-off-by: David Windsor <dwind...@gmail.com>
> >>> ---
> >>>  drivers/xen/gntdev.c | 11 ++-
> >>>  1 file changed, 6 insertions(+), 5 deletions(-)
> >> Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
> > Is there a tree that can take this change? Turns out it is better to 
> > propagate
> changes via separate trees and only leftovers can be taken via Greg's tree.
> >
> 
> Sure, we can take it via Xen tree for rc3.

Thank you very much!

Best Regards,
Elena.

> 
> -boris

RE: [PATCH 22/29] drivers, scsi: convert iscsi_task.refcount from atomic_t to refcount_t

[Reshetova, Elena]

> On Mon, Mar 06, 2017 at 04:21:09PM +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.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> 
> This looks OK to me.
> 
> Acked-by: Chris Leech 

Thank you for review! Do you have a tree that can take this change? 

Best Regards,
Elena.

> 
> > ---
> >  drivers/scsi/libiscsi.c| 8 
> >  drivers/scsi/qedi/qedi_iscsi.c | 2 +-
> >  include/scsi/libiscsi.h| 3 ++-
> >  3 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> > index 834d121..7eb1d2c 100644
> > --- a/drivers/scsi/libiscsi.c
> > +++ b/drivers/scsi/libiscsi.c
> > @@ -516,13 +516,13 @@ static void iscsi_free_task(struct iscsi_task *task)
> >
> >  void __iscsi_get_task(struct iscsi_task *task)
> >  {
> > -   atomic_inc(>refcount);
> > +   refcount_inc(>refcount);
> >  }
> >  EXPORT_SYMBOL_GPL(__iscsi_get_task);
> >
> >  void __iscsi_put_task(struct iscsi_task *task)
> >  {
> > -   if (atomic_dec_and_test(>refcount))
> > +   if (refcount_dec_and_test(>refcount))
> > iscsi_free_task(task);
> >  }
> >  EXPORT_SYMBOL_GPL(__iscsi_put_task);
> > @@ -744,7 +744,7 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct
> iscsi_hdr *hdr,
> >  * released by the lld when it has transmitted the task for
> >  * pdus we do not expect a response for.
> >  */
> > -   atomic_set(>refcount, 1);
> > +   refcount_set(>refcount, 1);
> > task->conn = conn;
> > task->sc = NULL;
> > INIT_LIST_HEAD(>running);
> > @@ -1616,7 +1616,7 @@ static inline struct iscsi_task 
> > *iscsi_alloc_task(struct
> iscsi_conn *conn,
> > sc->SCp.phase = conn->session->age;
> > sc->SCp.ptr = (char *) task;
> >
> > -   atomic_set(>refcount, 1);
> > +   refcount_set(>refcount, 1);
> > task->state = ISCSI_TASK_PENDING;
> > task->conn = conn;
> > task->sc = sc;
> > diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
> > index b9f79d3..3895bd5 100644
> > --- a/drivers/scsi/qedi/qedi_iscsi.c
> > +++ b/drivers/scsi/qedi/qedi_iscsi.c
> > @@ -1372,7 +1372,7 @@ static void qedi_cleanup_task(struct iscsi_task *task)
> >  {
> > if (!task->sc || task->state == ISCSI_TASK_PENDING) {
> > QEDI_INFO(NULL, QEDI_LOG_IO, "Returning
> ref_cnt=%d\n",
> > - atomic_read(>refcount));
> > + refcount_read(>refcount));
> > return;
> > }
> >
> > diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> > index b0e275d..24d74b5 100644
> > --- a/include/scsi/libiscsi.h
> > +++ b/include/scsi/libiscsi.h
> > @@ -29,6 +29,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -139,7 +140,7 @@ struct iscsi_task {
> >
> > /* state set/tested under session->lock */
> > int state;
> > -   atomic_trefcount;
> > +   refcount_t  refcount;
> > struct list_headrunning;/* running cmd list */
> > void*dd_data;   /*
> driver/transport data */
> >  };
> > --
> > 2.7.4
> >

RE: [PATCH 21/29] drivers, s390: convert fc_fcp_pkt.ref_cnt from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/06/2017 03:21 PM, 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.
> 
> The subject is wrong, should be something like "scsi: libfc convert
> fc_fcp_pkt.ref_cnt from atomic_t to refcount_t" but not s390.
> 
> Other than that
> Acked-by: Johannes Thumshirn 

Turns out that it is better that all these patches go through the respective 
maintainer trees, if present. 
If I send an updated patch (with subject fixed), could you merge it through 
your tree?

Best Regards,
Elena.
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

RE: [Xen-devel] [PATCH 29/29] drivers, xen: convert grant_map.users from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/06/2017 09:21 AM, 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.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  drivers/xen/gntdev.c | 11 ++-
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Boris Ostrovsky 

Is there a tree that can take this change? Turns out it is better to propagate 
changes via separate trees and only leftovers can be taken via Greg's tree.  

Best Regards,
Elena.

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 10/29] drivers, md: convert stripe_head.count from atomic_t to refcount_t

[Reshetova, Elena]

> On Mon, Mar 06, 2017 at 04:20:57PM +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.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  drivers/md/raid5-cache.c |  8 +++---
> >  drivers/md/raid5.c   | 66 
> > 
> >  drivers/md/raid5.h   |  3 ++-
> >  3 files changed, 39 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> > index 3f307be..6c05e12 100644
> > --- a/drivers/md/raid5-cache.c
> > +++ b/drivers/md/raid5-cache.c
> 
> snip
> >sh->check_state, sh->reconstruct_state);
> >
> > analyse_stripe(sh, );
> > @@ -4924,7 +4924,7 @@ static void activate_bit_delay(struct r5conf *conf,
> > struct stripe_head *sh = list_entry(head.next, struct
> stripe_head, lru);
> > int hash;
> > list_del_init(>lru);
> > -   atomic_inc(>count);
> > +   refcount_inc(>count);
> > hash = sh->hash_lock_index;
> > __release_stripe(conf, sh,
> _inactive_list[hash]);
> > }
> > @@ -5240,7 +5240,7 @@ static struct stripe_head 
> > *__get_priority_stripe(struct
> r5conf *conf, int group)
> > sh->group = NULL;
> > }
> > list_del_init(>lru);
> > -   BUG_ON(atomic_inc_return(>count) != 1);
> > +   BUG_ON(refcount_inc_not_zero(>count));
> 
> This changes the behavior. refcount_inc_not_zero doesn't inc if original 
> value is 0

Hm.. So, you want to inc here in any case and BUG if the end result differs 
from 1. 
So essentially you want to only increment here from zero to one under normal 
conditions... This is a challenge for refcount_t and against the design.
Is it ok just to maybe do this here:

-   BUG_ON(atomic_inc_return(>count) != 1);
+   BUG_ON(refcount_read(>count) != 0);
+   refcount_set((>count, 1);

Do we have an issue with locking in this case? Or maybe it is then better to 
leave this one to be atomic_t without protection since it isn't a real 
refcounter as it turns out. 

Best Regards,
Elena. 

> 
> Thanks,
> Shaohua

RE: [PATCH 13/29] drivers, media: convert vb2_vmarea_handler.refcount from atomic_t to refcount_t

[Reshetova, Elena]

> Hi Elena,
> 
> On Mon, Mar 06, 2017 at 04:21:00PM +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.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  drivers/media/v4l2-core/videobuf2-memops.c | 6 +++---
> >  include/media/videobuf2-memops.h   | 3 ++-
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/videobuf2-memops.c 
> > b/drivers/media/v4l2-
> core/videobuf2-memops.c
> > index 1cd322e..4bb8424 100644
> > --- a/drivers/media/v4l2-core/videobuf2-memops.c
> > +++ b/drivers/media/v4l2-core/videobuf2-memops.c
> > @@ -96,10 +96,10 @@ static void vb2_common_vm_open(struct
> vm_area_struct *vma)
> > struct vb2_vmarea_handler *h = vma->vm_private_data;
> >
> > pr_debug("%s: %p, refcount: %d, vma: %08lx-%08lx\n",
> > -  __func__, h, atomic_read(h->refcount), vma->vm_start,
> > +  __func__, h, refcount_read(h->refcount), vma->vm_start,
> >vma->vm_end);
> >
> > -   atomic_inc(h->refcount);
> > +   refcount_inc(h->refcount);
> >  }
> >
> >  /**
> > @@ -114,7 +114,7 @@ static void vb2_common_vm_close(struct
> vm_area_struct *vma)
> > struct vb2_vmarea_handler *h = vma->vm_private_data;
> >
> > pr_debug("%s: %p, refcount: %d, vma: %08lx-%08lx\n",
> > -  __func__, h, atomic_read(h->refcount), vma->vm_start,
> > +  __func__, h, refcount_read(h->refcount), vma->vm_start,
> >vma->vm_end);
> >
> > h->put(h->arg);
> > diff --git a/include/media/videobuf2-memops.h b/include/media/videobuf2-
> memops.h
> > index 36565c7a..a6ed091 100644
> > --- a/include/media/videobuf2-memops.h
> > +++ b/include/media/videobuf2-memops.h
> > @@ -16,6 +16,7 @@
> >
> >  #include 
> >  #include 
> > +#include 
> >
> >  /**
> >   * struct vb2_vmarea_handler - common vma refcount tracking handler
> > @@ -25,7 +26,7 @@
> >   * @arg:   argument for @put callback
> >   */
> >  struct vb2_vmarea_handler {
> > -   atomic_t*refcount;
> > +   refcount_t  *refcount;
> 
> This is a pointer to refcount, not refcount itself. The refcount is part of
> a memory type specific struct, the types that you change in the following
> three patches. I guess it would still compile and work as separate patches
> but you'd sure get warnings at least.

Actually it doesn't compile without this patch if I remember it correctly back 
in past when I was initially splitting the patches per variable. 

> 
> How about merging this and the three following patches that change the memop
> refcount types?

Sounds good!

Best Regards,
Elena.
> 
> > void(*put)(void *arg);
> > void*arg;
> >  };
> 
> --
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi   XMPP: sai...@retiisi.org.uk

RE: [PATCH 12/29] drivers, media: convert s2255_dev.num_channels from atomic_t to refcount_t

[Reshetova, Elena]

> Hi Elena,
> 
> On Mon, Mar 06, 2017 at 04:20:59PM +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.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> ...
> > @@ -1688,7 +1689,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev)
> > "failed to register
> video device!\n");
> > break;
> > }
> > -   atomic_inc(>num_channels);
> > +   refcount_set(>num_channels, 1);
> 
> That's not right. The loop runs four iterations and the value after the
> loop should be indeed the number of channels.

Oh, yes, I was blind here, sorry. The problem why it cannot be left as inc is 
because it would do increment from zero here, which is not allowed by 
refcount_t interface. 

> atomic_t isn't really used for reference counting here, but storing out how
> many "channels" there are per hardware device, with maximum number of four
> (4). I'd leave this driver using atomic_t.
Yes, sounds like the best thing to do. I will drop this patch. 

Thank you for reviews!

Best Regards,
Elena.
> 
> > v4l2_info(>v4l2_dev, "V4L2 device registered
> as %s\n",
> >   video_device_node_name(
> >vdev));
> >
> 
> --
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi   XMPP: sai...@retiisi.org.uk

RE: [PATCH 21/29] drivers, s390: convert fc_fcp_pkt.ref_cnt from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/06/2017 03:21 PM, 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.
> 
> The subject is wrong, should be something like "scsi: libfc convert
> fc_fcp_pkt.ref_cnt from atomic_t to refcount_t" but not s390.

Very sorry about this: the error in the subject got from the time when I was 
trying to break the bigger drivers patch set into per-variable part and trying 
to automate the process too much :(
I will fix it in the end version!

Best Regards,
Elena

> 
> Other than that
> Acked-by: Johannes Thumshirn 
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

RE: [PATCH 11/29] drivers, media: convert cx88_core.refcount from atomic_t to refcount_t

[Reshetova, Elena]

> Hello.
> 
> On 03/06/2017 05:20 PM, 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.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> [...]
> > diff --git a/drivers/media/pci/cx88/cx88.h b/drivers/media/pci/cx88/cx88.h
> > index 115414c..16c1313 100644
> > --- a/drivers/media/pci/cx88/cx88.h
> > +++ b/drivers/media/pci/cx88/cx88.h
> > @@ -24,6 +24,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include 
> >  #include 
> > @@ -339,7 +340,7 @@ struct cx8802_dev;
> >
> >  struct cx88_core {
> > struct list_head   devlist;
> > -   atomic_t   refcount;
> > +   refcount_t   refcount;
> 
> Could you please keep the name aligned with above and below?

You mean "not aligned" to devlist, but with a shift like it was before? 
Sure, will fix. Is the patch ok otherwise?

Best Regards,
Elena.

> 
> >
> > /* board name */
> > intnr;
> >
> 
> MBR, Sergei