| Message ID | 1487588781-15123-2-git-send-email-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 20, 2017 at 01:06:18PM +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.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> > --- > arch/s390/include/asm/debug.h | 3 ++- > arch/s390/kernel/debug.c | 8 ++++---- > 2 files changed, 6 insertions(+), 5 deletions(-) I can only see a pull request from Ingo a couple of hours ago for Peter's refcount code. So the refcount code is not merged yet. It would have been good if you would have waited until it is really merged to avoid confusion. > @@ -361,7 +361,7 @@ debug_info_create(const char *name, int pages_per_area, int nr_areas, > debug_area_last = rc; > rc->next = NULL; > > - debug_info_get(rc); > + refcount_set(&rc->ref_count, 1); This is not wrong, but I will remove this hunk before applying your patch, since this doesn't look like an obvious correct change at first glance. Thanks, Heiko
On Mon, Feb 20, 2017 at 02:24:24PM +0100, Heiko Carstens wrote: > > @@ -361,7 +361,7 @@ debug_info_create(const char *name, int pages_per_area, int nr_areas, > > debug_area_last = rc; > > rc->next = NULL; > > > > - debug_info_get(rc); > > + refcount_set(&rc->ref_count, 1); > > This is not wrong, but I will remove this hunk before applying your patch, > since this doesn't look like an obvious correct change at first glance. Actually your version is needed - just looked at refcount_inc(). Sorry for the confusion in my side now.
> On Mon, Feb 20, 2017 at 01:06:18PM +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.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> > > --- > > arch/s390/include/asm/debug.h | 3 ++- > > arch/s390/kernel/debug.c | 8 ++++---- > > 2 files changed, 6 insertions(+), 5 deletions(-) > > I can only see a pull request from Ingo a couple of hours ago for Peter's > refcount code. So the refcount code is not merged yet. It would have been > good if you would have waited until it is really merged to avoid confusion. Sorry, I guess I was a bit too rushy, but I also want to be able to fix all things that come up as I post these before next merge window closes. > > > @@ -361,7 +361,7 @@ debug_info_create(const char *name, int > pages_per_area, int nr_areas, > > debug_area_last = rc; > > rc->next = NULL; > > > > - debug_info_get(rc); > > + refcount_set(&rc->ref_count, 1); > > This is not wrong, but I will remove this hunk before applying your patch, > since this doesn't look like an obvious correct change at first glance. It isn't obvious, but needed unfortunately. refcount_inc is done in the way that it won't increment on zero value. And since for this variable you set the initial refcounter value to zero and then call debug_info_get (that does inc), this would only WARN and not increment. So for this initial case, we changed it to call refcount_set to "1" to make sure things work as before. Best Regards, Elena.
On Mon, Feb 20, 2017 at 02:24:24PM +0100, Heiko Carstens wrote: > On Mon, Feb 20, 2017 at 01:06:18PM +0200, Elena Reshetova wrote: > > @@ -361,7 +361,7 @@ debug_info_create(const char *name, int pages_per_area, int nr_areas, > > debug_area_last = rc; > > rc->next = NULL; > > > > - debug_info_get(rc); > > + refcount_set(&rc->ref_count, 1); > > This is not wrong, but I will remove this hunk before applying your patch, > since this doesn't look like an obvious correct change at first glance. I suspect; but have not looked at the code; that this would otherwise attempt to do a 0 -> 1 increment, which refcount_inc() will refuse (and WARN) over.
diff --git a/arch/s390/include/asm/debug.h b/arch/s390/include/asm/debug.h index 0206c80..df7b54e 100644 --- a/arch/s390/include/asm/debug.h +++ b/arch/s390/include/asm/debug.h @@ -10,6 +10,7 @@ #include <linux/spinlock.h> #include <linux/kernel.h> #include <linux/time.h> +#include <linux/refcount.h> #include <uapi/asm/debug.h> #define DEBUG_MAX_LEVEL 6 /* debug levels range from 0 to 6 */ @@ -31,7 +32,7 @@ struct debug_view; typedef struct debug_info { struct debug_info* next; struct debug_info* prev; - atomic_t ref_count; + refcount_t ref_count; spinlock_t lock; int level; int nr_areas; diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c index 20a5a42..e4b9929 100644 --- a/arch/s390/kernel/debug.c +++ b/arch/s390/kernel/debug.c @@ -277,7 +277,7 @@ debug_info_alloc(const char *name, int pages_per_area, int nr_areas, memset(rc->views, 0, DEBUG_MAX_VIEWS * sizeof(struct debug_view *)); memset(rc->debugfs_entries, 0 ,DEBUG_MAX_VIEWS * sizeof(struct dentry*)); - atomic_set(&(rc->ref_count), 0); + refcount_set(&(rc->ref_count), 0); return rc; @@ -361,7 +361,7 @@ debug_info_create(const char *name, int pages_per_area, int nr_areas, debug_area_last = rc; rc->next = NULL; - debug_info_get(rc); + refcount_set(&rc->ref_count, 1); out: return rc; } @@ -416,7 +416,7 @@ static void debug_info_get(debug_info_t * db_info) { if (db_info) - atomic_inc(&db_info->ref_count); + refcount_inc(&db_info->ref_count); } /* @@ -431,7 +431,7 @@ debug_info_put(debug_info_t *db_info) if (!db_info) return; - if (atomic_dec_and_test(&db_info->ref_count)) { + if (refcount_dec_and_test(&db_info->ref_count)) { for (i = 0; i < DEBUG_MAX_VIEWS; i++) { if (!db_info->views[i]) continue;