| Message ID | 20180703200141.28415-4-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018-07-03 22:01:38 [+0200], To linux-kernel@vger.kernel.org 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. Andrew, is it okay for you to collect this one (and 4/6 of this series, both bdi)? The prerequisites are already in Linus' tree. > Cc: linux-mm@kvack.org > Cc: Andrew Morton <akpm@linux-foundation.org> > Suggested-by: Peter Zijlstra <peterz@infradead.org> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > include/linux/backing-dev-defs.h | 3 ++- > include/linux/backing-dev.h | 4 ++-- > mm/backing-dev.c | 12 ++++++------ > 3 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h > index 24251762c20c..9a6bc0951cfa 100644 > --- a/include/linux/backing-dev-defs.h > +++ b/include/linux/backing-dev-defs.h > @@ -12,6 +12,7 @@ > #include <linux/timer.h> > #include <linux/workqueue.h> > #include <linux/kref.h> > +#include <linux/refcount.h> > > struct page; > struct device; > @@ -75,7 +76,7 @@ enum wb_reason { > */ > struct bdi_writeback_congested { > unsigned long state; /* WB_[a]sync_congested flags */ > - atomic_t refcnt; /* nr of attached wb's and blkg */ > + refcount_t refcnt; /* nr of attached wb's and blkg */ > > #ifdef CONFIG_CGROUP_WRITEBACK > struct backing_dev_info *__bdi; /* the associated bdi, set to NULL > diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h > index 72ca0f3d39f3..c28a47cbe355 100644 > --- a/include/linux/backing-dev.h > +++ b/include/linux/backing-dev.h > @@ -404,13 +404,13 @@ static inline bool inode_cgwb_enabled(struct inode *inode) > static inline struct bdi_writeback_congested * > wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp) > { > - atomic_inc(&bdi->wb_congested->refcnt); > + refcount_inc(&bdi->wb_congested->refcnt); > return bdi->wb_congested; > } > > static inline void wb_congested_put(struct bdi_writeback_congested *congested) > { > - if (atomic_dec_and_test(&congested->refcnt)) > + if (refcount_dec_and_test(&congested->refcnt)) > kfree(congested); > } > > diff --git a/mm/backing-dev.c b/mm/backing-dev.c > index 2e5d3df0853d..55a233d75f39 100644 > --- a/mm/backing-dev.c > +++ b/mm/backing-dev.c > @@ -438,10 +438,10 @@ wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp) > if (new_congested) { > /* !found and storage for new one already allocated, insert */ > congested = new_congested; > - new_congested = NULL; > rb_link_node(&congested->rb_node, parent, node); > rb_insert_color(&congested->rb_node, &bdi->cgwb_congested_tree); > - goto found; > + spin_unlock_irqrestore(&cgwb_lock, flags); > + return congested; > } > > spin_unlock_irqrestore(&cgwb_lock, flags); > @@ -451,13 +451,13 @@ wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp) > if (!new_congested) > return NULL; > > - atomic_set(&new_congested->refcnt, 0); > + refcount_set(&new_congested->refcnt, 1); > new_congested->__bdi = bdi; > new_congested->blkcg_id = blkcg_id; > goto retry; > > found: > - atomic_inc(&congested->refcnt); > + refcount_inc(&congested->refcnt); > spin_unlock_irqrestore(&cgwb_lock, flags); > kfree(new_congested); > return congested; > @@ -474,7 +474,7 @@ void wb_congested_put(struct bdi_writeback_congested *congested) > unsigned long flags; > > local_irq_save(flags); > - if (!atomic_dec_and_lock(&congested->refcnt, &cgwb_lock)) { > + if (!refcount_dec_and_lock(&congested->refcnt, &cgwb_lock)) { > local_irq_restore(flags); > return; > } > @@ -804,7 +804,7 @@ static int cgwb_bdi_init(struct backing_dev_info *bdi) > if (!bdi->wb_congested) > return -ENOMEM; > > - atomic_set(&bdi->wb_congested->refcnt, 1); > + refcount_set(&bdi->wb_congested->refcnt, 1); > > err = wb_init(&bdi->wb, bdi, 1, GFP_KERNEL); > if (err) { > -- > 2.18.0 Sebastian
On Tue, 3 Jul 2018 22:01:38 +0200 Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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. > > ... > > --- a/mm/backing-dev.c > +++ b/mm/backing-dev.c > @@ -438,10 +438,10 @@ wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp) > if (new_congested) { > /* !found and storage for new one already allocated, insert */ > congested = new_congested; > - new_congested = NULL; > rb_link_node(&congested->rb_node, parent, node); > rb_insert_color(&congested->rb_node, &bdi->cgwb_congested_tree); > - goto found; > + spin_unlock_irqrestore(&cgwb_lock, flags); > + return congested; > } > > spin_unlock_irqrestore(&cgwb_lock, flags); > @@ -451,13 +451,13 @@ wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp) > if (!new_congested) > return NULL; > > - atomic_set(&new_congested->refcnt, 0); > + refcount_set(&new_congested->refcnt, 1); > new_congested->__bdi = bdi; > new_congested->blkcg_id = blkcg_id; > goto retry; > > found: > - atomic_inc(&congested->refcnt); > + refcount_inc(&congested->refcnt); > spin_unlock_irqrestore(&cgwb_lock, flags); > kfree(new_congested); > return congested; > > ... > I'm not sure that the restructuring of wb_congested_get_create() was desirable and it does make the patch harder to review. But it looks OK to me.
On 2018-07-16 15:57:16 [-0700], Andrew Morton wrote: > > --- a/mm/backing-dev.c > > +++ b/mm/backing-dev.c > > @@ -438,10 +438,10 @@ wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp) > > if (new_congested) { > > /* !found and storage for new one already allocated, insert */ > > congested = new_congested; > > - new_congested = NULL; > > rb_link_node(&congested->rb_node, parent, node); > > rb_insert_color(&congested->rb_node, &bdi->cgwb_congested_tree); > > - goto found; > > + spin_unlock_irqrestore(&cgwb_lock, flags); > > + return congested; > > } > > > > spin_unlock_irqrestore(&cgwb_lock, flags); > > @@ -451,13 +451,13 @@ wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp) > > if (!new_congested) > > return NULL; > > > > - atomic_set(&new_congested->refcnt, 0); > > + refcount_set(&new_congested->refcnt, 1); > > new_congested->__bdi = bdi; > > new_congested->blkcg_id = blkcg_id; > > goto retry; > > > > found: > > - atomic_inc(&congested->refcnt); > > + refcount_inc(&congested->refcnt); > > spin_unlock_irqrestore(&cgwb_lock, flags); > > kfree(new_congested); > > return congested; > > > > ... > > > > I'm not sure that the restructuring of wb_congested_get_create() was > desirable and it does make the patch harder to review. But it looks > OK to me. By `restructuring' you mean the addition of return statement instead using the goto label in the first hunk? If so, then you would have refcount_set(&new_congested->refcnt, 0); refcount_inc(&congested->refcnt); which is a 0 -> 1 transition and is forbidden by refcount_t. So I had to avoid this one. Thank you applying the patches! You applied the bdi and userns switch from atomic_t to refcount_t. There were also the patches [PATCH 4/6] bdi: Use irqsave variant of refcount_dec_and_lock() [PATCH 6/6] userns: Use irqsave variant of refcount_dec_and_lock() in the series which make use the irqsave version of refcount_dec_and_lock(). Did you miss them by chance or skipped them on purpose? Sebastian
diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h index 24251762c20c..9a6bc0951cfa 100644 --- a/include/linux/backing-dev-defs.h +++ b/include/linux/backing-dev-defs.h @@ -12,6 +12,7 @@ #include <linux/timer.h> #include <linux/workqueue.h> #include <linux/kref.h> +#include <linux/refcount.h> struct page; struct device; @@ -75,7 +76,7 @@ enum wb_reason { */ struct bdi_writeback_congested { unsigned long state; /* WB_[a]sync_congested flags */ - atomic_t refcnt; /* nr of attached wb's and blkg */ + refcount_t refcnt; /* nr of attached wb's and blkg */ #ifdef CONFIG_CGROUP_WRITEBACK struct backing_dev_info *__bdi; /* the associated bdi, set to NULL diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h index 72ca0f3d39f3..c28a47cbe355 100644 --- a/include/linux/backing-dev.h +++ b/include/linux/backing-dev.h @@ -404,13 +404,13 @@ static inline bool inode_cgwb_enabled(struct inode *inode) static inline struct bdi_writeback_congested * wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp) { - atomic_inc(&bdi->wb_congested->refcnt); + refcount_inc(&bdi->wb_congested->refcnt); return bdi->wb_congested; } static inline void wb_congested_put(struct bdi_writeback_congested *congested) { - if (atomic_dec_and_test(&congested->refcnt)) + if (refcount_dec_and_test(&congested->refcnt)) kfree(congested); } diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 2e5d3df0853d..55a233d75f39 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -438,10 +438,10 @@ wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp) if (new_congested) { /* !found and storage for new one already allocated, insert */ congested = new_congested; - new_congested = NULL; rb_link_node(&congested->rb_node, parent, node); rb_insert_color(&congested->rb_node, &bdi->cgwb_congested_tree); - goto found; + spin_unlock_irqrestore(&cgwb_lock, flags); + return congested; } spin_unlock_irqrestore(&cgwb_lock, flags); @@ -451,13 +451,13 @@ wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp) if (!new_congested) return NULL; - atomic_set(&new_congested->refcnt, 0); + refcount_set(&new_congested->refcnt, 1); new_congested->__bdi = bdi; new_congested->blkcg_id = blkcg_id; goto retry; found: - atomic_inc(&congested->refcnt); + refcount_inc(&congested->refcnt); spin_unlock_irqrestore(&cgwb_lock, flags); kfree(new_congested); return congested; @@ -474,7 +474,7 @@ void wb_congested_put(struct bdi_writeback_congested *congested) unsigned long flags; local_irq_save(flags); - if (!atomic_dec_and_lock(&congested->refcnt, &cgwb_lock)) { + if (!refcount_dec_and_lock(&congested->refcnt, &cgwb_lock)) { local_irq_restore(flags); return; } @@ -804,7 +804,7 @@ static int cgwb_bdi_init(struct backing_dev_info *bdi) if (!bdi->wb_congested) return -ENOMEM; - atomic_set(&bdi->wb_congested->refcnt, 1); + refcount_set(&bdi->wb_congested->refcnt, 1); err = wb_init(&bdi->wb, bdi, 1, GFP_KERNEL); if (err) {