Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1585438 > unrolled thread
| Started by | Elena Reshetova <elena.reshetova@intel.com> |
|---|---|
| First post | 2017-02-21 16:50 +0100 |
| Last post | 2017-02-21 17:00 +0100 |
| Articles | 4 on this page of 24 — 6 participants |
Back to article view | Back to linux.kernel
[PATCH 0/7] fs, xfs subsystem refcounter conversions Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:50 +0100
[PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 16:50 +0100
Re: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-02-21 17:40 +0100
RE: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-22 12:20 +0100
Re: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t Dave Chinner <david@fromorbit.com> - 2017-02-22 00:00 +0100
RE: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-22 12:30 +0100
Re: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t Dave Chinner <david@fromorbit.com> - 2017-02-22 23:10 +0100
RE: [PATCH 1/7] fs, xfs: convert xfs_bui_log_item.bui_refcount from atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-23 09:00 +0100
[PATCH 5/7] fs, xfs: convert xlog_ticket.t_ref from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 17:00 +0100
Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-02-21 17:00 +0100
RE: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-21 17:10 +0100
Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-02-21 17:30 +0100
Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-02-21 17:40 +0100
Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-02-21 18:10 +0100
Re: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t Brian Foster <bfoster@redhat.com> - 2017-02-21 20:30 +0100
RE: [PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-22 12:30 +0100
[PATCH 7/7] fs, xfs: convert xfs_rui_log_item.rui_refcount from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 17:00 +0100
[PATCH 4/7] fs, xfs: convert xfs_efi_log_item.efi_refcount from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 17:00 +0100
[PATCH 3/7] fs, xfs: convert xfs_buf_log_item.bli_refcount from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 17:00 +0100
[PATCH 2/7] fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 17:00 +0100
Re: [PATCH 2/7] fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-02-21 17:10 +0100
Re: [PATCH 2/7] fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to refcount_t Dave Chinner <david@fromorbit.com> - 2017-02-22 00:00 +0100
RE: [PATCH 2/7] fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-22 12:20 +0100
[PATCH 6/7] fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-02-21 17:00 +0100
Page 2 of 2 — ← Prev page 1 [2]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2017-02-21 17:10 +0100 |
| Subject | Re: [PATCH 2/7] fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to refcount_t |
| Message-ID | <tdlaN-4Vr-5@gated-at.bofh.it> |
| In reply to | #1585471 |
On Tue, Feb 21, 2017 at 05:49:02PM +0200, Elena Reshetova wrote:
> @@ -1684,10 +1684,11 @@ xfs_buftarg_isolate(
> * zero. If the value is already zero, we need to reclaim the
> * buffer, otherwise it gets another trip through the LRU.
> */
> - if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) {
> + if (!refcount_read(&bp->b_lru_ref)) {
> spin_unlock(&bp->b_lock);
> return LRU_ROTATE;
> }
> + refcount_dec_and_test(&bp->b_lru_ref);
>
> bp->b_state |= XFS_BSTATE_DISPOSE;
> list_lru_isolate_move(lru, item, dispose);
This should never have passed testing.. refcount_dec_and_test() has a
__must_check.
Furthermore the above seems to suggest thingies can live with a 0
refcount, so a straight conversion cannot work.
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2017-02-22 00:00 +0100 |
| Subject | Re: [PATCH 2/7] fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to refcount_t |
| Message-ID | <tdrzz-FS-15@gated-at.bofh.it> |
| In reply to | #1585475 |
On Tue, Feb 21, 2017 at 05:04:08PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 21, 2017 at 05:49:02PM +0200, Elena Reshetova wrote:
> > @@ -1684,10 +1684,11 @@ xfs_buftarg_isolate(
> > * zero. If the value is already zero, we need to reclaim the
> > * buffer, otherwise it gets another trip through the LRU.
> > */
> > - if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) {
> > + if (!refcount_read(&bp->b_lru_ref)) {
> > spin_unlock(&bp->b_lock);
> > return LRU_ROTATE;
> > }
> > + refcount_dec_and_test(&bp->b_lru_ref);
> >
> > bp->b_state |= XFS_BSTATE_DISPOSE;
> > list_lru_isolate_move(lru, item, dispose);
>
> This should never have passed testing.. refcount_dec_and_test() has a
> __must_check.
>
> Furthermore the above seems to suggest thingies can live with a 0
> refcount, so a straight conversion cannot work.
Yes, 0 is a valid value - the buffer lru reference is *not an object
lifecycle reference count*. A value of zero means reclaim needs to
take action if it sees that value - it does not mean that the object
is not referenced by anyone (that's b_hold). i.e. b_lru_ref is an
"active reference weighting" used to provide a heirarchical reclaim
bias toward less important metadata objects, and has no bearing on
the actual active users of the object.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | "Reshetova, Elena" <elena.reshetova@intel.com> |
|---|---|
| Date | 2017-02-22 12:20 +0100 |
| Subject | RE: [PATCH 2/7] fs, xfs: convert xfs_buf.b_hold and xfs_buf.b_lru_ref from atomic_t to refcount_t |
| Message-ID | <tdD7I-R8-21@gated-at.bofh.it> |
| In reply to | #1585790 |
> On Tue, Feb 21, 2017 at 05:04:08PM +0100, Peter Zijlstra wrote:
> > On Tue, Feb 21, 2017 at 05:49:02PM +0200, Elena Reshetova wrote:
> > > @@ -1684,10 +1684,11 @@ xfs_buftarg_isolate(
> > > * zero. If the value is already zero, we need to reclaim the
> > > * buffer, otherwise it gets another trip through the LRU.
> > > */
> > > - if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) {
> > > + if (!refcount_read(&bp->b_lru_ref)) {
> > > spin_unlock(&bp->b_lock);
> > > return LRU_ROTATE;
> > > }
> > > + refcount_dec_and_test(&bp->b_lru_ref);
> > >
> > > bp->b_state |= XFS_BSTATE_DISPOSE;
> > > list_lru_isolate_move(lru, item, dispose);
> >
> > This should never have passed testing.. refcount_dec_and_test() has a
> > __must_check.
> >
> > Furthermore the above seems to suggest thingies can live with a 0
> > refcount, so a straight conversion cannot work.
>
> Yes, 0 is a valid value - the buffer lru reference is *not an object
> lifecycle reference count*. A value of zero means reclaim needs to
> take action if it sees that value - it does not mean that the object
> is not referenced by anyone (that's b_hold). i.e. b_lru_ref is an
> "active reference weighting" used to provide a heirarchical reclaim
> bias toward less important metadata objects, and has no bearing on
> the actual active users of the object.
OK, so all of this suggests that we should not conver b_lru_ref to the refcount_t then.
I will remove this conversion from this commit and only leave b_hold.
Thank you!
Best Regards,
Elena.
[toc] | [prev] | [next] | [standalone]
| From | Elena Reshetova <elena.reshetova@intel.com> |
|---|---|
| Date | 2017-02-21 17:00 +0100 |
| Subject | [PATCH 6/7] fs, xfs: convert xfs_cui_log_item.cui_refcount from atomic_t to refcount_t |
| Message-ID | <tdl1a-4zS-53@gated-at.bofh.it> |
| In reply to | #1585438 |
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@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
fs/xfs/xfs_refcount_item.c | 4 ++--
fs/xfs/xfs_refcount_item.h | 4 +++-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 6e4c744..61bc570 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -205,7 +205,7 @@ xfs_cui_init(
cuip->cui_format.cui_nextents = nextents;
cuip->cui_format.cui_id = (uintptr_t)(void *)cuip;
atomic_set(&cuip->cui_next_extent, 0);
- atomic_set(&cuip->cui_refcount, 2);
+ refcount_set(&cuip->cui_refcount, 2);
return cuip;
}
@@ -221,7 +221,7 @@ void
xfs_cui_release(
struct xfs_cui_log_item *cuip)
{
- if (atomic_dec_and_test(&cuip->cui_refcount)) {
+ if (refcount_dec_and_test(&cuip->cui_refcount)) {
xfs_trans_ail_remove(&cuip->cui_item, SHUTDOWN_LOG_IO_ERROR);
xfs_cui_item_free(cuip);
}
diff --git a/fs/xfs/xfs_refcount_item.h b/fs/xfs/xfs_refcount_item.h
index 5b74ddd..7f23ff8 100644
--- a/fs/xfs/xfs_refcount_item.h
+++ b/fs/xfs/xfs_refcount_item.h
@@ -20,6 +20,8 @@
#ifndef __XFS_REFCOUNT_ITEM_H__
#define __XFS_REFCOUNT_ITEM_H__
+#include <linux/refcount.h>
+
/*
* There are (currently) two pairs of refcount btree redo item types:
* increase and decrease. The log items for these are CUI (refcount
@@ -63,7 +65,7 @@ struct kmem_zone;
*/
struct xfs_cui_log_item {
struct xfs_log_item cui_item;
- atomic_t cui_refcount;
+ refcount_t cui_refcount;
atomic_t cui_next_extent;
unsigned long cui_flags; /* misc flags */
struct xfs_cui_log_format cui_format;
--
2.7.4
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web