Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1521929 > unrolled thread

[RFC][PATCH 0/7] kref improvements

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH 0/7] kref improvements Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
    [RFC][PATCH 5/7] kref: Implement kref_put_lock() Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
      Re: [RFC][PATCH 5/7] kref: Implement kref_put_lock() Kees Cook <keescook@chromium.org> - 2016-11-14 21:40 +0100
        Re: [RFC][PATCH 5/7] kref: Implement kref_put_lock() Peter Zijlstra <peterz@infradead.org> - 2016-11-15 09:00 +0100
    [RFC][PATCH 6/7] kref: Avoid more abuse Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
    [RFC][PATCH 4/7] kref: Use kref_get_unless_zero() more Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
    [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-15 09:50 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 10:50 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-15 11:10 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 11:50 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-15 14:10 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-15 19:10 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 20:20 +0100
                    Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-15 20:30 +0100
                      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-16 09:40 +0100
                        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 10:00 +0100
                          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-16 10:10 +0100
                            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 10:30 +0100
                        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-16 11:20 +0100
                          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-16 20:00 +0100
                            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 09:40 +0100
                              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-17 21:00 +0100
                        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-16 19:50 +0100
      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-15 13:40 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 14:10 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-15 15:20 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 10:30 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-17 10:50 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 11:40 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 11:50 +0100
                    Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Greg KH <gregkh@linuxfoundation.org> - 2016-11-17 12:10 +0100
                      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:10 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:30 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-17 18:10 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-18 09:30 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-18 11:20 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-17 18:10 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:30 +0100
      RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-18 11:10 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 12:40 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-18 18:10 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 20:00 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 05:10 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-21 08:50 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 09:40 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 09:50 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-21 10:10 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 10:40 +0100
      RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-18 11:50 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 12:00 +0100
          RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-18 18:00 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 20:00 +0100
              RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-19 08:20 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-19 12:50 +0100
    Re: [RFC][PATCH 2/7] kref: Add kref_read() Christoph Hellwig <hch@infradead.org> - 2016-11-14 19:20 +0100
      Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 08:30 +0100
        Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-15 08:50 +0100
        [PATCH] printk, locking/atomics, kref: Introduce new %pAr and %pAk  format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-15 09:40 +0100
          [PATCH v2] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-15 09:50 +0100
            Re: [PATCH v2] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Peter Zijlstra <peterz@infradead.org> - 2016-11-15 10:30 +0100
              [PATCH v3] printk, locking/atomics, kref: Introduce new %pAa and  %pAk format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-15 10:50 +0100
            Re: [PATCH v2] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' kbuild test robot <lkp@intel.com> - 2016-11-15 11:10 +0100
          Re: [PATCH] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-15 17:50 +0100
            Re: [PATCH] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-16 09:20 +0100
    Re: [RFC][PATCH 0/7] kref improvements Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 08:30 +0100
      Re: [RFC][PATCH 0/7] kref improvements Ingo Molnar <mingo@kernel.org> - 2016-11-15 08:50 +0100
        Re: [RFC][PATCH 0/7] kref improvements Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 16:10 +0100
      Re: [RFC][PATCH 0/7] kref improvements Peter Zijlstra <peterz@infradead.org> - 2016-11-15 08:50 +0100
    Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 08:40 +0100
      Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-15 09:10 +0100
        Re: [RFC][PATCH 2/7] kref: Add kref_read() Kees Cook <keescook@chromium.org> - 2016-11-15 22:00 +0100
          Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 09:30 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-16 11:20 +0100
              Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 11:20 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 11:20 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Daniel Borkmann <daniel@iogearbox.net> - 2016-11-16 11:20 +0100
          Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-16 11:10 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Kees Cook <keescook@chromium.org> - 2016-11-16 20:00 +0100
              Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 09:40 +0100
                Re: [RFC][PATCH 2/7] kref: Add kref_read() David Windsor <dave@progbits.org> - 2016-11-17 13:50 +0100
                  RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-17 15:40 +0100
                    Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:10 +0100
                      RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-17 18:10 +0100
                      RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-17 19:10 +0100
                        Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 20:20 +0100
                        Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 20:40 +0100
                  Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:20 +0100
                Re: [RFC][PATCH 2/7] kref: Add kref_read() Kees Cook <keescook@chromium.org> - 2016-11-17 20:40 +0100

Page 4 of 5 — ← Prev page 1 2 3 [4] 5  Next page →

#1522483 — Re: [PATCH v2] printk, locking/atomics, kref: Introduce new %pAr and %pAk format string options for atomic_t and 'struct kref'

On Tue, Nov 15, 2016 at 09:43:55AM +0100, Ingo Molnar wrote:
> +atomic variables such atomic_t or struct kref:
> +
> +	%pAr	atomic_t count

Why 'r' for atomic_t ? I was expecting 'a' for atomic_t or something.
That then also leaves 'r' available for refcount_t.

> +	%pAk	struct kref count

[toc] | [prev] | [next] | [standalone]

#1522498 — [PATCH v3] printk, locking/atomics, kref: Introduce new %pAa and %pAk format string options for atomic_t and 'struct kref'

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Nov 15, 2016 at 09:43:55AM +0100, Ingo Molnar wrote:
> > +atomic variables such atomic_t or struct kref:
> > +
> > +	%pAr	atomic_t count
> 
> Why 'r' for atomic_t ? I was expecting 'a' for atomic_t or something.
> That then also leaves 'r' available for refcount_t.

'r' was for 'raw atomic count', but you are right - new patch attached below.

Thanks,

	Ingo

==========
Subject: printk, locking/atomics, kref: Introduce new %pAa and %pAk format string options for atomic_t and 'struct kref'
From: Ingo Molnar <mingo@kernel.org>
Date: Tue, 15 Nov 2016 09:43:55 +0100

A decade of kref internals exposed to driver writers has proven that
exposing internals to them is a bad idea.

Make the bad patterns a bit easier to detect and allow cleaner
printouts by offering two new printk format string extensions:

	%pAa - print the atomic_t count in decimal
	%pAk - print the struct kref count in decimal

Also add printf testcases:

  [    0.334919] test_printf: all 268 tests passed

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: arnd@arndb.de
Cc: dave@progbits.org
Cc: elena.reshetova@intel.com
Cc: keescook@chromium.org
Cc: will.deacon@arm.com
Link: http://lkml.kernel.org/r/20161115084355.GA24175@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 Documentation/printk-formats.txt |   10 +++++++++
 lib/test_printf.c                |   28 ++++++++++++++++++++++++++
 lib/vsprintf.c                   |   42 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+)

Index: tip/Documentation/printk-formats.txt
===================================================================
--- tip.orig/Documentation/printk-formats.txt
+++ tip/Documentation/printk-formats.txt
@@ -316,6 +316,16 @@ Flags bitfields such as page flags, gfp_
 
 	Passed by reference.
 
+atomic variables such atomic_t or struct kref:
+
+	%pAa	atomic_t count
+	%pAk	struct kref count
+
+	For printing the current count value of atomic variables. This is
+	preferred to accessing the counts directly.
+
+	Passed by reference.
+
 Network device features:
 
 	%pNF	0x000000000000c000
Index: tip/lib/test_printf.c
===================================================================
--- tip.orig/lib/test_printf.c
+++ tip/lib/test_printf.c
@@ -20,6 +20,8 @@
 #include <linux/gfp.h>
 #include <linux/mm.h>
 
+#include <linux/kref.h>
+
 #define BUF_SIZE 256
 #define PAD_SIZE 16
 #define FILL_CHAR '$'
@@ -462,6 +464,31 @@ flags(void)
 	kfree(cmp_buffer);
 }
 
+/*
+ * Testcases for %pAa (atomic_t) and %pAk (struct kref) count printing:
+ */
+static void __init test_atomics__atomic_t(void)
+{
+	atomic_t count = ATOMIC_INIT(1);
+
+	test("1", "%pAa", &count);
+}
+
+static void __init test_atomics__kref(void)
+{
+	struct kref kref;
+
+	kref_init(&kref);
+
+	test("1", "%pAk", &kref);
+}
+
+static void __init test_atomics(void)
+{
+	test_atomics__atomic_t();
+	test_atomics__kref();
+}
+
 static void __init
 test_pointer(void)
 {
@@ -481,6 +508,7 @@ test_pointer(void)
 	bitmap();
 	netdev_features();
 	flags();
+	test_atomics();
 }
 
 static int __init
Index: tip/lib/vsprintf.c
===================================================================
--- tip.orig/lib/vsprintf.c
+++ tip/lib/vsprintf.c
@@ -38,6 +38,8 @@
 
 #include "../mm/internal.h"	/* For the trace_print_flags arrays */
 
+#include <linux/kref.h>
+
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/sections.h>	/* for dereference_function_descriptor() */
 #include <asm/byteorder.h>	/* cpu_to_le16 */
@@ -1470,6 +1472,40 @@ char *flags_string(char *buf, char *end,
 	return format_flags(buf, end, flags, names);
 }
 
+static noinline_for_stack
+char *atomic_var(char *buf, char *end, void *atomic_ptr, const char *fmt)
+{
+	unsigned long num;
+	const struct printf_spec numspec = {
+		.flags		= SPECIAL|SMALL,
+		.field_width	= -1,
+		.precision	= -1,
+		.base		= 10,
+	};
+
+	switch (fmt[1]) {
+		case 'a':
+		{
+			atomic_t *count_p = (void *)atomic_ptr;
+
+			num = atomic_read(count_p);
+			break;
+		}
+		case 'k':
+		{
+			struct kref *kref_p = (void *)atomic_ptr;
+
+			num = refcount_read(&kref_p->refcount);
+			break;
+		}
+		default:
+			WARN_ONCE(1, "Unsupported atomics modifier: %c\n", fmt[1]);
+			return buf;
+	}
+
+	return number(buf, end, num, numspec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1563,6 +1599,10 @@ int kptr_restrict __read_mostly;
  *       p page flags (see struct page) given as pointer to unsigned long
  *       g gfp flags (GFP_* and __GFP_*) given as pointer to gfp_t
  *       v vma flags (VM_*) given as pointer to unsigned long
+ * - 'A' For the count of atomic variables to be printed.
+ *       Supported flags given by option:
+ *        a atomic_t    ('a'tomic count)
+ *        k struct kref ('k'ref count)
  *
  * ** Please update also Documentation/printk-formats.txt when making changes **
  *
@@ -1718,6 +1758,8 @@ char *pointer(const char *fmt, char *buf
 
 	case 'G':
 		return flags_string(buf, end, ptr, fmt);
+	case 'A':
+		return atomic_var(buf, end, ptr, fmt);
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {

[toc] | [prev] | [next] | [standalone]

#1522518 — Re: [PATCH v2] printk, locking/atomics, kref: Introduce new %pAr and %pAk format string options for atomic_t and 'struct kref'

[Multipart message — attachments visible in raw view] — view raw

Hi Ingo,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.9-rc5 next-20161115]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ingo-Molnar/printk-locking-atomics-kref-Introduce-new-pAr-and-pAk-format-string-options-for-atomic_t-and-struct-kref/20161115-174900
config: i386-randconfig-x006-201646 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   lib/vsprintf.c: In function 'atomic_var':
>> lib/vsprintf.c:1498:10: error: implicit declaration of function 'refcount_read' [-Werror=implicit-function-declaration]
       num = refcount_read(&kref_p->refcount);
             ^~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/refcount_read +1498 lib/vsprintf.c

  1492				break;
  1493			}
  1494			case 'k':
  1495			{
  1496				struct kref *kref_p = (void *)atomic_ptr;
  1497	
> 1498				num = refcount_read(&kref_p->refcount);
  1499				break;
  1500			}
  1501			default:

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1522910 — Re: [PATCH] printk, locking/atomics, kref: Introduce new %pAr and %pAk format string options for atomic_t and 'struct kref'

On Tue, Nov 15, 2016 at 12:37 AM, Ingo Molnar <mingo@kernel.org> wrote:
> +atomic variables such atomic_t or struct kref:
> +
> +       %pAr    atomic_t count
> +       %pAk    struct kref count

Not a huge fan. That "r" makes sense to you ("raw" atomic), but it
makes no sense to a user. An atomic isn't "raw" to anybody else. It's
just an atomic.

Also, we have 'atomic64_t", which this doesn't cover at all.

I'd suggest just %pA, %pA64, %pAkref or something. Which leaves us the
choice to add more atomic versions later without having to make up
random one-letter things that make no sense.

           Linus

[toc] | [prev] | [next] | [standalone]

#1523280 — Re: [PATCH] printk, locking/atomics, kref: Introduce new %pAr and %pAk format string options for atomic_t and 'struct kref'

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, Nov 15, 2016 at 12:37 AM, Ingo Molnar <mingo@kernel.org> wrote:
> > +atomic variables such atomic_t or struct kref:
> > +
> > +       %pAr    atomic_t count
> > +       %pAk    struct kref count
> 
> Not a huge fan. That "r" makes sense to you ("raw" atomic), but it
> makes no sense to a user. An atomic isn't "raw" to anybody else. It's
> just an atomic.

So in the latestest patch this has evolved to:

        %pAa - print the 'atomic_t'    count in decimal
        %pAk - print the 'struct kref' count in decimal
        %pAr - print the 'refcount_t'  count in decimal

... are you still hating it?

> Also, we have 'atomic64_t", which this doesn't cover at all.

We could use a somewhat logical letter for atomic64_t too:

	%pAA - print the 'atomic64_t'  count in decimal

... as 'A' is the bigger version of 'a', just like atomic64_t is the bigger 
version of atomic_t! ;-)

> I'd suggest just %pA, %pA64, %pAkref or something. Which leaves us the
> choice to add more atomic versions later without having to make up
> random one-letter things that make no sense.

It's a bit more work, but we could do that too, if you still don't like the above 
single letter abbreviations.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1522401

On Mon, Nov 14, 2016 at 06:39:46PM +0100, Peter Zijlstra wrote:
> This series unfscks kref and then implements it in terms of refcount_t.
> 
> x86_64-allyesconfig compile tested and boot tested with my regular config.
> 
> refcount_t is as per the previous thread, it BUGs on over-/underflow and
> saturates at UINT_MAX, such that if we ever overflow, we'll never free again.
> 
> 

Thanks so much for doing these, at the very least, I want to take the
kref-abuse-fixes now as those users shouldn't be doing those foolish
things.  Any objection for me taking some of them through my tree now?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1522413

* Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, Nov 14, 2016 at 06:39:46PM +0100, Peter Zijlstra wrote:
> > This series unfscks kref and then implements it in terms of refcount_t.
> > 
> > x86_64-allyesconfig compile tested and boot tested with my regular config.
> > 
> > refcount_t is as per the previous thread, it BUGs on over-/underflow and
> > saturates at UINT_MAX, such that if we ever overflow, we'll never free again.
> > 
> > 
> 
> Thanks so much for doing these, at the very least, I want to take the
> kref-abuse-fixes now as those users shouldn't be doing those foolish
> things.  Any objection for me taking some of them through my tree now?

Very nice series indeed!

We normally route atomics related patches through tip:locking/core (there's also 
tip:atomic/core), but this is a special case I think, given how broadly it 
interacts with driver code.

So both would work I think: we could concentrate these and only these patches into 
tip:atomic/core into an append-only tree, or you can carry them in the driver tree 
- whichever variant you prefer!

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1522800

On Tue, Nov 15, 2016 at 08:42:45AM +0100, Ingo Molnar wrote:
> * Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Nov 14, 2016 at 06:39:46PM +0100, Peter Zijlstra wrote:
> > Thanks so much for doing these, at the very least, I want to take the
> > kref-abuse-fixes now as those users shouldn't be doing those foolish
> > things.  Any objection for me taking some of them through my tree now?
> 
> Very nice series indeed!
> 
> We normally route atomics related patches through tip:locking/core (there's also 
> tip:atomic/core), but this is a special case I think, given how broadly it 
> interacts with driver code.
> 
> So both would work I think: we could concentrate these and only these patches into 
> tip:atomic/core into an append-only tree, or you can carry them in the driver tree 
> - whichever variant you prefer!

I have no objection for them going through a tip:whataver tree/branch,
if you all want to do that, it makes it easier for me :)

I'm guessing another series will come out before they get merged there,
so I'll hold off on providing an Acked-by: until then.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1522415

On Tue, Nov 15, 2016 at 08:27:42AM +0100, Greg KH wrote:
> On Mon, Nov 14, 2016 at 06:39:46PM +0100, Peter Zijlstra wrote:
> > This series unfscks kref and then implements it in terms of refcount_t.
> > 
> > x86_64-allyesconfig compile tested and boot tested with my regular config.
> > 
> > refcount_t is as per the previous thread, it BUGs on over-/underflow and
> > saturates at UINT_MAX, such that if we ever overflow, we'll never free again.
> > 
> > 
> 
> Thanks so much for doing these, at the very least, I want to take the
> kref-abuse-fixes now as those users shouldn't be doing those foolish
> things.  Any objection for me taking some of them through my tree now?

None at all, but please double check at least the 'kill kref_sub()' one,
I might have messed up drbd or something, that code isn't entirely
transparant.

[toc] | [prev] | [next] | [standalone]

#1522411 — Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Mon, Nov 14, 2016 at 06:39:48PM +0100, Peter Zijlstra wrote:
> Since we need to change the implementation, stop exposing internals.
> 
> Provide kref_read() to read the current reference count; typically
> used for debug messages.
> 
> Kills two anti-patterns:
> 
> 	atomic_read(&kref->refcount)
> 	kref->refcount.counter
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  drivers/block/drbd/drbd_req.c                |    2 -
>  drivers/block/rbd.c                          |    8 ++---
>  drivers/block/virtio_blk.c                   |    2 -
>  drivers/gpu/drm/drm_gem_cma_helper.c         |    2 -
>  drivers/gpu/drm/drm_info.c                   |    2 -
>  drivers/gpu/drm/drm_mode_object.c            |    4 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c        |    2 -
>  drivers/gpu/drm/msm/msm_gem.c                |    2 -
>  drivers/gpu/drm/nouveau/nouveau_fence.c      |    2 -
>  drivers/gpu/drm/omapdrm/omap_gem.c           |    2 -
>  drivers/gpu/drm/ttm/ttm_bo.c                 |    4 +-
>  drivers/gpu/drm/ttm/ttm_object.c             |    2 -
>  drivers/infiniband/hw/cxgb3/iwch_cm.h        |    6 ++--
>  drivers/infiniband/hw/cxgb3/iwch_qp.c        |    2 -
>  drivers/infiniband/hw/cxgb4/iw_cxgb4.h       |    6 ++--
>  drivers/infiniband/hw/cxgb4/qp.c             |    2 -
>  drivers/infiniband/hw/usnic/usnic_ib_sysfs.c |    6 ++--
>  drivers/infiniband/hw/usnic/usnic_ib_verbs.c |    4 +-
>  drivers/misc/genwqe/card_dev.c               |    2 -
>  drivers/misc/mei/debugfs.c                   |    2 -
>  drivers/pci/hotplug/pnv_php.c                |    2 -
>  drivers/pci/slot.c                           |    2 -
>  drivers/scsi/bnx2fc/bnx2fc_io.c              |    8 ++---
>  drivers/scsi/cxgbi/libcxgbi.h                |    4 +-
>  drivers/scsi/lpfc/lpfc_debugfs.c             |    2 -
>  drivers/scsi/lpfc/lpfc_els.c                 |    2 -
>  drivers/scsi/lpfc/lpfc_hbadisc.c             |   40 +++++++++++++--------------
>  drivers/scsi/lpfc/lpfc_init.c                |    3 --
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c           |    4 +-
>  drivers/staging/android/ion/ion.c            |    2 -
>  drivers/staging/comedi/comedi_buf.c          |    2 -
>  drivers/target/target_core_pr.c              |   10 +++---
>  drivers/target/tcm_fc/tfc_sess.c             |    2 -
>  drivers/usb/gadget/function/f_fs.c           |    2 -
>  fs/exofs/sys.c                               |    2 -
>  fs/ocfs2/cluster/netdebug.c                  |    2 -
>  fs/ocfs2/cluster/tcp.c                       |    2 -
>  fs/ocfs2/dlm/dlmdebug.c                      |   12 ++++----
>  fs/ocfs2/dlm/dlmdomain.c                     |    2 -
>  fs/ocfs2/dlm/dlmmaster.c                     |    8 ++---
>  fs/ocfs2/dlm/dlmunlock.c                     |    2 -
>  include/drm/drm_framebuffer.h                |    2 -
>  include/drm/ttm/ttm_bo_driver.h              |    4 +-
>  include/linux/kref.h                         |    5 +++
>  include/linux/sunrpc/cache.h                 |    2 -
>  include/net/bluetooth/hci_core.h             |    4 +-
>  net/bluetooth/6lowpan.c                      |    2 -
>  net/bluetooth/a2mp.c                         |    4 +-
>  net/bluetooth/amp.c                          |    4 +-
>  net/bluetooth/l2cap_core.c                   |    4 +-
>  net/ceph/messenger.c                         |    4 +-
>  net/ceph/osd_client.c                        |   10 +++---
>  net/sunrpc/cache.c                           |    2 -
>  net/sunrpc/svc_xprt.c                        |    6 ++--
>  net/sunrpc/xprtrdma/svc_rdma_transport.c     |    4 +-
>  55 files changed, 120 insertions(+), 116 deletions(-)
> 
> --- a/drivers/block/drbd/drbd_req.c
> +++ b/drivers/block/drbd/drbd_req.c
> @@ -520,7 +520,7 @@ static void mod_rq_state(struct drbd_req
>  		/* Completion does it's own kref_put.  If we are going to
>  		 * kref_sub below, we need req to be still around then. */
>  		int at_least = k_put + !!c_put;
> -		int refcount = atomic_read(&req->kref.refcount);
> +		int refcount = kref_read(&req->kref);
>  		if (refcount < at_least)
>  			drbd_err(device,
>  				"mod_rq_state: Logic BUG: %x -> %x: refcount = %d, should be >= %d\n",

As proof of "things you should never do", here is one such example.

ugh.


> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -767,7 +767,7 @@ static void virtblk_remove(struct virtio
>  	/* Stop all the virtqueues. */
>  	vdev->config->reset(vdev);
>  
> -	refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount);
> +	refc = kref_read(&disk_to_dev(vblk->disk)->kobj.kref);
>  	put_disk(vblk->disk);
>  	vdev->config->del_vqs(vdev);
>  	kfree(vblk->vqs);

And this too, ugh, that's a huge abuse and is probably totally wrong...

thanks again for digging through this crap.  I wonder if we need to name
the kref reference variable "do_not_touch_this_ever" or some such thing
to catch all of the people who try to be "too smart".

greg k-h

[toc] | [prev] | [next] | [standalone]

#1522422 — Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Tue, Nov 15, 2016 at 08:33:22AM +0100, Greg KH wrote:
> On Mon, Nov 14, 2016 at 06:39:48PM +0100, Peter Zijlstra wrote:

> > --- a/drivers/block/drbd/drbd_req.c
> > +++ b/drivers/block/drbd/drbd_req.c
> > @@ -520,7 +520,7 @@ static void mod_rq_state(struct drbd_req
> >  		/* Completion does it's own kref_put.  If we are going to
> >  		 * kref_sub below, we need req to be still around then. */
> >  		int at_least = k_put + !!c_put;
> > -		int refcount = atomic_read(&req->kref.refcount);
> > +		int refcount = kref_read(&req->kref);
> >  		if (refcount < at_least)
> >  			drbd_err(device,
> >  				"mod_rq_state: Logic BUG: %x -> %x: refcount = %d, should be >= %d\n",
> 
> As proof of "things you should never do", here is one such example.
> 
> ugh.
> 
> 
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -767,7 +767,7 @@ static void virtblk_remove(struct virtio
> >  	/* Stop all the virtqueues. */
> >  	vdev->config->reset(vdev);
> >  
> > -	refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount);
> > +	refc = kref_read(&disk_to_dev(vblk->disk)->kobj.kref);
> >  	put_disk(vblk->disk);
> >  	vdev->config->del_vqs(vdev);
> >  	kfree(vblk->vqs);
> 
> And this too, ugh, that's a huge abuse and is probably totally wrong...
> 
> thanks again for digging through this crap.  I wonder if we need to name
> the kref reference variable "do_not_touch_this_ever" or some such thing
> to catch all of the people who try to be "too smart".

There's unimaginable bong hits involved in this stuff, in the end I
resorted to brute force and scripts to convert all this.

[toc] | [prev] | [next] | [standalone]

#1523059 — Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Tue, Nov 15, 2016 at 12:03 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Nov 15, 2016 at 08:33:22AM +0100, Greg KH wrote:
>> On Mon, Nov 14, 2016 at 06:39:48PM +0100, Peter Zijlstra wrote:
>
>> > --- a/drivers/block/drbd/drbd_req.c
>> > +++ b/drivers/block/drbd/drbd_req.c
>> > @@ -520,7 +520,7 @@ static void mod_rq_state(struct drbd_req
>> >             /* Completion does it's own kref_put.  If we are going to
>> >              * kref_sub below, we need req to be still around then. */
>> >             int at_least = k_put + !!c_put;
>> > -           int refcount = atomic_read(&req->kref.refcount);
>> > +           int refcount = kref_read(&req->kref);
>> >             if (refcount < at_least)
>> >                     drbd_err(device,
>> >                             "mod_rq_state: Logic BUG: %x -> %x: refcount = %d, should be >= %d\n",
>>
>> As proof of "things you should never do", here is one such example.
>>
>> ugh.
>>
>>
>> > --- a/drivers/block/virtio_blk.c
>> > +++ b/drivers/block/virtio_blk.c
>> > @@ -767,7 +767,7 @@ static void virtblk_remove(struct virtio
>> >     /* Stop all the virtqueues. */
>> >     vdev->config->reset(vdev);
>> >
>> > -   refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount);
>> > +   refc = kref_read(&disk_to_dev(vblk->disk)->kobj.kref);
>> >     put_disk(vblk->disk);
>> >     vdev->config->del_vqs(vdev);
>> >     kfree(vblk->vqs);
>>
>> And this too, ugh, that's a huge abuse and is probably totally wrong...
>>
>> thanks again for digging through this crap.  I wonder if we need to name
>> the kref reference variable "do_not_touch_this_ever" or some such thing
>> to catch all of the people who try to be "too smart".
>
> There's unimaginable bong hits involved in this stuff, in the end I
> resorted to brute force and scripts to convert all this.

What should we do about things like this (bpf_prog_put() and callbacks
from kernel/bpf/syscall.c):


static void bpf_prog_uncharge_memlock(struct bpf_prog *prog)
{
        struct user_struct *user = prog->aux->user;

        atomic_long_sub(prog->pages, &user->locked_vm);
        free_uid(user);
}

static void __bpf_prog_put_rcu(struct rcu_head *rcu)
{
        struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);

        free_used_maps(aux);
        bpf_prog_uncharge_memlock(aux->prog);
        bpf_prog_free(aux->prog);
}

void bpf_prog_put(struct bpf_prog *prog)
{
        if (atomic_dec_and_test(&prog->aux->refcnt))
                call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
}


Not only do we want to protect prog->aux->refcnt, but I think we want
to protect user->locked_vm too ... I don't think it's sane for
user->locked_vm to be a stats_t ?

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1523289 — Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Tue, Nov 15, 2016 at 12:53:35PM -0800, Kees Cook wrote:
> On Tue, Nov 15, 2016 at 12:03 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Tue, Nov 15, 2016 at 08:33:22AM +0100, Greg KH wrote:
> >> On Mon, Nov 14, 2016 at 06:39:48PM +0100, Peter Zijlstra wrote:
> >
> >> > --- a/drivers/block/drbd/drbd_req.c
> >> > +++ b/drivers/block/drbd/drbd_req.c
> >> > @@ -520,7 +520,7 @@ static void mod_rq_state(struct drbd_req
> >> >             /* Completion does it's own kref_put.  If we are going to
> >> >              * kref_sub below, we need req to be still around then. */
> >> >             int at_least = k_put + !!c_put;
> >> > -           int refcount = atomic_read(&req->kref.refcount);
> >> > +           int refcount = kref_read(&req->kref);
> >> >             if (refcount < at_least)
> >> >                     drbd_err(device,
> >> >                             "mod_rq_state: Logic BUG: %x -> %x: refcount = %d, should be >= %d\n",
> >>
> >> As proof of "things you should never do", here is one such example.
> >>
> >> ugh.
> >>
> >>
> >> > --- a/drivers/block/virtio_blk.c
> >> > +++ b/drivers/block/virtio_blk.c
> >> > @@ -767,7 +767,7 @@ static void virtblk_remove(struct virtio
> >> >     /* Stop all the virtqueues. */
> >> >     vdev->config->reset(vdev);
> >> >
> >> > -   refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount);
> >> > +   refc = kref_read(&disk_to_dev(vblk->disk)->kobj.kref);
> >> >     put_disk(vblk->disk);
> >> >     vdev->config->del_vqs(vdev);
> >> >     kfree(vblk->vqs);
> >>
> >> And this too, ugh, that's a huge abuse and is probably totally wrong...
> >>
> >> thanks again for digging through this crap.  I wonder if we need to name
> >> the kref reference variable "do_not_touch_this_ever" or some such thing
> >> to catch all of the people who try to be "too smart".
> >
> > There's unimaginable bong hits involved in this stuff, in the end I
> > resorted to brute force and scripts to convert all this.
> 
> What should we do about things like this (bpf_prog_put() and callbacks
> from kernel/bpf/syscall.c):
> 
> 
> static void bpf_prog_uncharge_memlock(struct bpf_prog *prog)
> {
>         struct user_struct *user = prog->aux->user;
> 
>         atomic_long_sub(prog->pages, &user->locked_vm);

Oh that's scary.  Let's just make one reference count rely on another
one and not check things...

>         free_uid(user);
> }
> 
> static void __bpf_prog_put_rcu(struct rcu_head *rcu)
> {
>         struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);
> 
>         free_used_maps(aux);
>         bpf_prog_uncharge_memlock(aux->prog);
>         bpf_prog_free(aux->prog);
> }
> 
> void bpf_prog_put(struct bpf_prog *prog)
> {
>         if (atomic_dec_and_test(&prog->aux->refcnt))
>                 call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
> }
> 
> 
> Not only do we want to protect prog->aux->refcnt, but I think we want
> to protect user->locked_vm too ... I don't think it's sane for
> user->locked_vm to be a stats_t ?

I don't think this is sane code...

greg k-h

[toc] | [prev] | [next] | [standalone]

#1523356 — Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Wed, Nov 16, 2016 at 09:21:51AM +0100, Greg KH wrote:
> > What should we do about things like this (bpf_prog_put() and callbacks
> > from kernel/bpf/syscall.c):
> > 
> > 
> > static void bpf_prog_uncharge_memlock(struct bpf_prog *prog)
> > {
> >         struct user_struct *user = prog->aux->user;
> > 
> >         atomic_long_sub(prog->pages, &user->locked_vm);
> 
> Oh that's scary.  Let's just make one reference count rely on another
> one and not check things...

Its not a reference count, its a resource limit thingy. Also, isn't
stacking, or in general building an object graph, the entire point of
reference counts?

> >         free_uid(user);
> > }
> > 
> > static void __bpf_prog_put_rcu(struct rcu_head *rcu)
> > {
> >         struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);
> > 
> >         free_used_maps(aux);
> >         bpf_prog_uncharge_memlock(aux->prog);
> >         bpf_prog_free(aux->prog);
> > }
> > 
> > void bpf_prog_put(struct bpf_prog *prog)
> > {
> >         if (atomic_dec_and_test(&prog->aux->refcnt))
> >                 call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
> > }
> > 
> > 
> > Not only do we want to protect prog->aux->refcnt, but I think we want
> > to protect user->locked_vm too ... I don't think it's sane for
> > user->locked_vm to be a stats_t ?
> 
> I don't think this is sane code...

I once again fail to see any problems. That code is fine.

[toc] | [prev] | [next] | [standalone]

#1523358 — Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Wed, Nov 16, 2016 at 11:10:42AM +0100, Peter Zijlstra wrote:
> On Wed, Nov 16, 2016 at 09:21:51AM +0100, Greg KH wrote:
> > > What should we do about things like this (bpf_prog_put() and callbacks
> > > from kernel/bpf/syscall.c):
> > > 
> > > 
> > > static void bpf_prog_uncharge_memlock(struct bpf_prog *prog)
> > > {
> > >         struct user_struct *user = prog->aux->user;
> > > 
> > >         atomic_long_sub(prog->pages, &user->locked_vm);
> > 
> > Oh that's scary.  Let's just make one reference count rely on another
> > one and not check things...
> 
> Its not a reference count, its a resource limit thingy. Also, isn't
> stacking, or in general building an object graph, the entire point of
> reference counts?

Ah, that wasn't obvious, but yes, you are correct here, sorry for the
noise.

greg k-h

[toc] | [prev] | [next] | [standalone]

#1523357 — Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Wed, Nov 16, 2016 at 11:11:43AM +0100, Daniel Borkmann wrote:
> On 11/16/2016 09:21 AM, Greg KH wrote:
> > On Tue, Nov 15, 2016 at 12:53:35PM -0800, Kees Cook wrote:
> > > On Tue, Nov 15, 2016 at 12:03 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > > On Tue, Nov 15, 2016 at 08:33:22AM +0100, Greg KH wrote:
> > > > > On Mon, Nov 14, 2016 at 06:39:48PM +0100, Peter Zijlstra wrote:
> > > > 
> > > > > > --- a/drivers/block/drbd/drbd_req.c
> > > > > > +++ b/drivers/block/drbd/drbd_req.c
> > > > > > @@ -520,7 +520,7 @@ static void mod_rq_state(struct drbd_req
> > > > > >              /* Completion does it's own kref_put.  If we are going to
> > > > > >               * kref_sub below, we need req to be still around then. */
> > > > > >              int at_least = k_put + !!c_put;
> > > > > > -           int refcount = atomic_read(&req->kref.refcount);
> > > > > > +           int refcount = kref_read(&req->kref);
> > > > > >              if (refcount < at_least)
> > > > > >                      drbd_err(device,
> > > > > >                              "mod_rq_state: Logic BUG: %x -> %x: refcount = %d, should be >= %d\n",
> > > > > 
> > > > > As proof of "things you should never do", here is one such example.
> > > > > 
> > > > > ugh.
> > > > > 
> > > > > > --- a/drivers/block/virtio_blk.c
> > > > > > +++ b/drivers/block/virtio_blk.c
> > > > > > @@ -767,7 +767,7 @@ static void virtblk_remove(struct virtio
> > > > > >      /* Stop all the virtqueues. */
> > > > > >      vdev->config->reset(vdev);
> > > > > > 
> > > > > > -   refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount);
> > > > > > +   refc = kref_read(&disk_to_dev(vblk->disk)->kobj.kref);
> > > > > >      put_disk(vblk->disk);
> > > > > >      vdev->config->del_vqs(vdev);
> > > > > >      kfree(vblk->vqs);
> > > > > 
> > > > > And this too, ugh, that's a huge abuse and is probably totally wrong...
> > > > > 
> > > > > thanks again for digging through this crap.  I wonder if we need to name
> > > > > the kref reference variable "do_not_touch_this_ever" or some such thing
> > > > > to catch all of the people who try to be "too smart".
> > > > 
> > > > There's unimaginable bong hits involved in this stuff, in the end I
> > > > resorted to brute force and scripts to convert all this.
> > > 
> > > What should we do about things like this (bpf_prog_put() and callbacks
> > > from kernel/bpf/syscall.c):
> 
> Just reading up on this series. Your question refers to converting bpf
> prog and map ref counts to Peter's refcount_t eventually, right?
> 
> > > static void bpf_prog_uncharge_memlock(struct bpf_prog *prog)
> > > {
> > >          struct user_struct *user = prog->aux->user;
> > > 
> > >          atomic_long_sub(prog->pages, &user->locked_vm);
> > 
> > Oh that's scary.  Let's just make one reference count rely on another
> > one and not check things...
> 
> Sorry, could you elaborate what you mean by 'check things', you mean for
> wrap around? IIUC, back then accounting was roughly similar modeled after
> perf event's one, and in this case accounts for pages used by progs and
> maps during their life-time. Are you suggesting that this approach is
> inherently broken?

No, it is correct, I responded too quickly before my morning coffee had
kicked in, my apologies.

greg k-h

[toc] | [prev] | [next] | [standalone]

#1523360 — Re: [RFC][PATCH 2/7] kref: Add kref_read()

On 11/16/2016 09:21 AM, Greg KH wrote:
> On Tue, Nov 15, 2016 at 12:53:35PM -0800, Kees Cook wrote:
>> On Tue, Nov 15, 2016 at 12:03 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>> On Tue, Nov 15, 2016 at 08:33:22AM +0100, Greg KH wrote:
>>>> On Mon, Nov 14, 2016 at 06:39:48PM +0100, Peter Zijlstra wrote:
>>>
>>>>> --- a/drivers/block/drbd/drbd_req.c
>>>>> +++ b/drivers/block/drbd/drbd_req.c
>>>>> @@ -520,7 +520,7 @@ static void mod_rq_state(struct drbd_req
>>>>>              /* Completion does it's own kref_put.  If we are going to
>>>>>               * kref_sub below, we need req to be still around then. */
>>>>>              int at_least = k_put + !!c_put;
>>>>> -           int refcount = atomic_read(&req->kref.refcount);
>>>>> +           int refcount = kref_read(&req->kref);
>>>>>              if (refcount < at_least)
>>>>>                      drbd_err(device,
>>>>>                              "mod_rq_state: Logic BUG: %x -> %x: refcount = %d, should be >= %d\n",
>>>>
>>>> As proof of "things you should never do", here is one such example.
>>>>
>>>> ugh.
>>>>
>>>>> --- a/drivers/block/virtio_blk.c
>>>>> +++ b/drivers/block/virtio_blk.c
>>>>> @@ -767,7 +767,7 @@ static void virtblk_remove(struct virtio
>>>>>      /* Stop all the virtqueues. */
>>>>>      vdev->config->reset(vdev);
>>>>>
>>>>> -   refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount);
>>>>> +   refc = kref_read(&disk_to_dev(vblk->disk)->kobj.kref);
>>>>>      put_disk(vblk->disk);
>>>>>      vdev->config->del_vqs(vdev);
>>>>>      kfree(vblk->vqs);
>>>>
>>>> And this too, ugh, that's a huge abuse and is probably totally wrong...
>>>>
>>>> thanks again for digging through this crap.  I wonder if we need to name
>>>> the kref reference variable "do_not_touch_this_ever" or some such thing
>>>> to catch all of the people who try to be "too smart".
>>>
>>> There's unimaginable bong hits involved in this stuff, in the end I
>>> resorted to brute force and scripts to convert all this.
>>
>> What should we do about things like this (bpf_prog_put() and callbacks
>> from kernel/bpf/syscall.c):

Just reading up on this series. Your question refers to converting bpf
prog and map ref counts to Peter's refcount_t eventually, right?

>> static void bpf_prog_uncharge_memlock(struct bpf_prog *prog)
>> {
>>          struct user_struct *user = prog->aux->user;
>>
>>          atomic_long_sub(prog->pages, &user->locked_vm);
>
> Oh that's scary.  Let's just make one reference count rely on another
> one and not check things...

Sorry, could you elaborate what you mean by 'check things', you mean for
wrap around? IIUC, back then accounting was roughly similar modeled after
perf event's one, and in this case accounts for pages used by progs and
maps during their life-time. Are you suggesting that this approach is
inherently broken?

>>          free_uid(user);
>> }
>>
>> static void __bpf_prog_put_rcu(struct rcu_head *rcu)
>> {
>>          struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);
>>
>>          free_used_maps(aux);
>>          bpf_prog_uncharge_memlock(aux->prog);
>>          bpf_prog_free(aux->prog);
>> }
>>
>> void bpf_prog_put(struct bpf_prog *prog)
>> {
>>          if (atomic_dec_and_test(&prog->aux->refcnt))
>>                  call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
>> }
>>
>>
>> Not only do we want to protect prog->aux->refcnt, but I think we want
>> to protect user->locked_vm too ... I don't think it's sane for
>> user->locked_vm to be a stats_t ?
>
> I don't think this is sane code...
>
> greg k-h

[toc] | [prev] | [next] | [standalone]

#1523354 — Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Tue, Nov 15, 2016 at 12:53:35PM -0800, Kees Cook wrote:
> 
> What should we do about things like this (bpf_prog_put() and callbacks
> from kernel/bpf/syscall.c):
> 
> 
> static void bpf_prog_uncharge_memlock(struct bpf_prog *prog)
> {
>         struct user_struct *user = prog->aux->user;
> 
>         atomic_long_sub(prog->pages, &user->locked_vm);
>         free_uid(user);
> }
> 
> static void __bpf_prog_put_rcu(struct rcu_head *rcu)
> {
>         struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);
> 
>         free_used_maps(aux);
>         bpf_prog_uncharge_memlock(aux->prog);
>         bpf_prog_free(aux->prog);
> }
> 
> void bpf_prog_put(struct bpf_prog *prog)
> {
>         if (atomic_dec_and_test(&prog->aux->refcnt))
>                 call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
> }
> 
> 
> Not only do we want to protect prog->aux->refcnt, but I think we want
> to protect user->locked_vm too ... I don't think it's sane for
> user->locked_vm to be a stats_t ?

Why would you want to mess with locked_vm? You seem of the opinion that
everything atomic_t is broken, this isn't the case.

[toc] | [prev] | [next] | [standalone]

#1523754 — Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Wed, Nov 16, 2016 at 2:09 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Nov 15, 2016 at 12:53:35PM -0800, Kees Cook wrote:
>>
>> What should we do about things like this (bpf_prog_put() and callbacks
>> from kernel/bpf/syscall.c):
>>
>>
>> static void bpf_prog_uncharge_memlock(struct bpf_prog *prog)
>> {
>>         struct user_struct *user = prog->aux->user;
>>
>>         atomic_long_sub(prog->pages, &user->locked_vm);
>>         free_uid(user);
>> }
>>
>> static void __bpf_prog_put_rcu(struct rcu_head *rcu)
>> {
>>         struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);
>>
>>         free_used_maps(aux);
>>         bpf_prog_uncharge_memlock(aux->prog);
>>         bpf_prog_free(aux->prog);
>> }
>>
>> void bpf_prog_put(struct bpf_prog *prog)
>> {
>>         if (atomic_dec_and_test(&prog->aux->refcnt))
>>                 call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
>> }
>>
>>
>> Not only do we want to protect prog->aux->refcnt, but I think we want
>> to protect user->locked_vm too ... I don't think it's sane for
>> user->locked_vm to be a stats_t ?
>
> Why would you want to mess with locked_vm? You seem of the opinion that
> everything atomic_t is broken, this isn't the case.

What I mean to say is that while the refcnt here should clearly be
converted to kref or refcount_t, it looks like locked_vm should become
a new stats_t. However, it seems weird for locked_vm to ever wrap
either...

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1524163 — Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Wed, Nov 16, 2016 at 10:58:38AM -0800, Kees Cook wrote:
> On Wed, Nov 16, 2016 at 2:09 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Tue, Nov 15, 2016 at 12:53:35PM -0800, Kees Cook wrote:
> >>
> >> What should we do about things like this (bpf_prog_put() and callbacks
> >> from kernel/bpf/syscall.c):
> >>
> >>
> >> static void bpf_prog_uncharge_memlock(struct bpf_prog *prog)
> >> {
> >>         struct user_struct *user = prog->aux->user;
> >>
> >>         atomic_long_sub(prog->pages, &user->locked_vm);
> >>         free_uid(user);
> >> }
> >>
> >> static void __bpf_prog_put_rcu(struct rcu_head *rcu)
> >> {
> >>         struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);
> >>
> >>         free_used_maps(aux);
> >>         bpf_prog_uncharge_memlock(aux->prog);
> >>         bpf_prog_free(aux->prog);
> >> }
> >>
> >> void bpf_prog_put(struct bpf_prog *prog)
> >> {
> >>         if (atomic_dec_and_test(&prog->aux->refcnt))
> >>                 call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
> >> }
> >>
> >>
> >> Not only do we want to protect prog->aux->refcnt, but I think we want
> >> to protect user->locked_vm too ... I don't think it's sane for
> >> user->locked_vm to be a stats_t ?
> >
> > Why would you want to mess with locked_vm? You seem of the opinion that
> > everything atomic_t is broken, this isn't the case.
> 
> What I mean to say is that while the refcnt here should clearly be
> converted to kref or refcount_t, it looks like locked_vm should become
> a new stats_t. However, it seems weird for locked_vm to ever wrap
> either...

No, its not a statistic. Also, I'm far from convinced stats_t is an
actually useful thing to have.

refcount_t brought special semantics that clearly are different from
regular atomic_t, stats_t would not, so why would it need to exist.

Not to mention that you seem over eager to apply it, which doesn't
inspire confidence.

[toc] | [prev] | [next] | [standalone]

Page 4 of 5 — ← Prev page 1 2 3 [4] 5  Next page →

Back to top | Article view | linux.kernel

csiph-web