LinuxLists.cc - [RFC][PATCH 2/7] kref: Add kref

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Mon, Nov 14, 2016 at 10:16:55AM -0800, Christoph Hellwig wrote:
> 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.
>
> Can we just provide a printk specifier for a kref value instead as
> that is the only valid use case for reading the value?

Yeah, that would be great as no one should be doing anything
logic-related based on the kref value.

thanks,

greg k-h

2016-11-15 07:33:16

[permalink] [raw]

Subject: 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) <[email protected]>
> ---
> 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

2016-11-15 07:47:15

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Tue, Nov 15, 2016 at 08:28:55AM +0100, Greg KH wrote:
> On Mon, Nov 14, 2016 at 10:16:55AM -0800, Christoph Hellwig wrote:
> > 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.
> >
> > Can we just provide a printk specifier for a kref value instead as
> > that is the only valid use case for reading the value?
>
> Yeah, that would be great as no one should be doing anything
> logic-related based on the kref value.

IIRC there are a few users that WARN_ON() the value with a minimum bound
or somesuch. Those would be left in the cold, but yes I too like the
idea of a printk() format specifier.

2016-11-15 08:03:18

[permalink] [raw]

Subject: 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.

2016-11-15 08:37:42

[permalink] [raw]

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

* Greg KH <[email protected]> wrote:

> On Mon, Nov 14, 2016 at 10:16:55AM -0800, Christoph Hellwig wrote:
> > 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.
> >
> > Can we just provide a printk specifier for a kref value instead as
> > that is the only valid use case for reading the value?
>
> Yeah, that would be great as no one should be doing anything
> logic-related based on the kref value.

Find below a patch that implements %pAk for 'struct kref' count printing and
%pAr for atomic_t counter printing.

This is against vanilla upstream.

Thanks,

Ingo

============================>
Subject: printk, locking/atomics, kref: Introduce new %pAr and %pAk format string options for atomic_t and 'struct kref'
From: Ingo Molnar <[email protected]>
Date: Tue Nov 15 08:53:14 CET 2016

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:

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

Also add printf testcases:

[ 0.353126] test_printf: all 268 tests passed

Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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:
+
+ %pAr 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 %pAr (atomic_t) and %pAk (struct kref) count printing:
+ */
+static void __init test_atomics__atomic_t(void)
+{
+ atomic_t count = ATOMIC_INIT(1);
+
+ test("1", "%pAr", &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 'r':
+ {
+ atomic_t *count_p = (void *)atomic_ptr;
+
+ num = atomic_read(count_p);
+ break;
+ }
+ case 'k':
+ {
+ struct kref *kref_p = (void *)atomic_ptr;
+
+ num = atomic_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:
+ * r atomic_t ('r'aw 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) {

2016-11-15 08:44:11

[permalink] [raw]

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

* Ingo Molnar <[email protected]> wrote:

>
> * Greg KH <[email protected]> wrote:
>
> > On Mon, Nov 14, 2016 at 10:16:55AM -0800, Christoph Hellwig wrote:
> > > 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.
> > >
> > > Can we just provide a printk specifier for a kref value instead as
> > > that is the only valid use case for reading the value?
> >
> > Yeah, that would be great as no one should be doing anything
> > logic-related based on the kref value.
>
> Find below a patch that implements %pAk for 'struct kref' count printing and
> %pAr for atomic_t counter printing.
>
> This is against vanilla upstream.

The patch below is against Peter's refcount series. Note that this patch depends
on this patch in Peter's series:

kref: Implement using refcount_t

Thanks,

Ingo

==================================>
Subject: printk, locking/atomics, kref: Introduce new %pAr and %pAk format string options for atomic_t and 'struct kref'
From: Ingo Molnar <[email protected]>
Date: Tue Nov 15 08:53:14 CET 2016

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:

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

Also add printf testcases:

[ 0.328495] test_printf: all 268 tests passed

Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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:
+
+ %pAr 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 %pAr (atomic_t) and %pAk (struct kref) count printing:
+ */
+static void __init test_atomics__atomic_t(void)
+{
+ atomic_t count = ATOMIC_INIT(1);
+
+ test("1", "%pAr", &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 'r':
+ {
+ 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:
+ * r atomic_t ('r'aw 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) {

2016-11-15 09:21:40

[permalink] [raw]

Subject: 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

2016-11-15 09:42:02

[permalink] [raw]

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

* Peter Zijlstra <[email protected]> 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 <[email protected]>
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 <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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) {

2016-11-15 10:04:10

by kernel test robot

[permalink] [raw]

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

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

Attachments:

(No filename) (1.29 kB)
.config.gz (27.22 kB)
Download all attachments

2016-11-15 16:42:44

by Linus Torvalds

[permalink] [raw]

Subject: 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 <[email protected]> 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

2016-11-15 20:53:42

by Kees Cook

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Tue, Nov 15, 2016 at 12:03 AM, Peter Zijlstra <[email protected]> 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

2016-11-16 08:14:03

[permalink] [raw]

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

* Linus Torvalds <[email protected]> wrote:

> On Tue, Nov 15, 2016 at 12:37 AM, Ingo Molnar <[email protected]> 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

2016-11-16 08:21:43

[permalink] [raw]

Subject: 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 <[email protected]> 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

2016-11-16 10:09:44

[permalink] [raw]

Subject: 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.

2016-11-16 10:11:01

[permalink] [raw]

Subject: 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.

2016-11-16 10:12:23

by Daniel Borkmann

[permalink] [raw]

Subject: 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 <[email protected]> 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

2016-11-16 10:18:28

[permalink] [raw]

Subject: 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

2016-11-16 10:19:06

[permalink] [raw]

Subject: 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 <[email protected]> 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

2016-11-16 18:58:42

by Kees Cook

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Wed, Nov 16, 2016 at 2:09 AM, Peter Zijlstra <[email protected]> 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

2016-11-16 20:09:15

by Alexei Starovoitov

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Wed, Nov 16, 2016 at 10:58 AM, Kees Cook <[email protected]> wrote:
> On Wed, Nov 16, 2016 at 2:09 AM, Peter Zijlstra <[email protected]> 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...

I prefer to avoid 'fixing' things that are not broken.
Note, prog->aux->refcnt already has explicit checks for overflow.
locked_vm is used for resource accounting and not refcnt,
so I don't see issues there either.

2016-11-17 08:35:08

[permalink] [raw]

Subject: 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 <[email protected]> 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.

2016-11-17 08:53:53

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Wed, Nov 16, 2016 at 12:08:52PM -0800, Alexei Starovoitov wrote:

> I prefer to avoid 'fixing' things that are not broken.
> Note, prog->aux->refcnt already has explicit checks for overflow.
> locked_vm is used for resource accounting and not refcnt,
> so I don't see issues there either.

The idea is to use something along the lines of:

http://lkml.kernel.org/r/[email protected]

for all refcounts in the kernel.

Also note that your:

struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
{
if (atomic_add_return(i, &prog->aux->refcnt) > BPF_MAX_REFCNT) {
atomic_sub(i, &prog->aux->refcnt);
return ERR_PTR(-EBUSY);
}
return prog;
}

is actually broken in the face of an actual overflow. Suppose @i is big
enough to wrap refcnt into negative space.

Also, the current sentiment is to strongly discourage add/sub operations
for refcounts.

2016-11-17 12:45:39

by David Windsor

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Thu, Nov 17, 2016 at 3:34 AM, Peter Zijlstra <[email protected]> wrote:
> On Wed, Nov 16, 2016 at 10:58:38AM -0800, Kees Cook wrote:
>> On Wed, Nov 16, 2016 at 2:09 AM, Peter Zijlstra <[email protected]> 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.
>

Regarding this, has there been any thought given as to how stats_t
will meaningfully differ from atomic_t? If refcount_t is semantically
"atomic_t with reference counter overflow protection," what
services/guarantees does stats_t provide? I cannot think of any that
don't require implementing overflow detection of some sort, which
incurs a performance hit.

One conceivable service/guarantee would be to give stats_t the ability
to detect/report when an overflow has occurred, but not ultimately
with the offending process getting killed. On x86, this could be
done by having stats_t overflows generate a different exception number
and corresponding handler than refcount_t-generated overflows. It
would still contain the mechanisms for detecting and responding to
overflows, but the response to stats_t overflows would differ from
that of refcount_t overflows. Semantically, this version of stats_t
would be "refcount_t minus 'kill the offending process'." I'm not
sure if this abstraction is in fact useful, or indeed worth the
requisite performance hit; I'm just suggesting a possible semantic
difference between atomic_t and stats_t.

> 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.

2016-11-17 14:30:38

[permalink] [raw]

Subject: RE: [RFC][PATCH 2/7] kref: Add kref_read()

On Thu, Nov 17, 2016 at 07:30:29AM -0500, David Windsor wrote:
> On Thu, Nov 17, 2016 at 3:34 AM, Peter Zijlstra <[email protected]> wrote:
> > No, its not a statistic. Also, I'm far from convinced stats_t is an
> > actually useful thing to have.
> >
>
> Regarding this, has there been any thought given as to how stats_t
> will meaningfully differ from atomic_t? If refcount_t is semantically
> "atomic_t with reference counter overflow protection," what
> services/guarantees does stats_t provide? I cannot think of any that
> don't require implementing overflow detection of some sort, which
> incurs a performance hit.

>Afaict the whole point of stats_t was to allow overflow, since its only stats, nobody cares etc..

>I think the sole motivator is a general distaste of atomic_t, which isn't a good reason at all.

I don't think anyone has this as motivation. But atomic_t is so powerful and flexible that easily ends up being misused (as past CVEs shown).
Even if we now find all occurrences of atomic_t used as refcounter (which we cannot actually guarantee in any case unless someone manually reads every line)
and convert it to refcount_t, we still have atomic_t type present and new usage of it as refount will crawl in. It is just a matter of time IMO.

So, this approach still doesn't solve the main problem: abuse of atomic_t a refcounter and security vulnerabilities as result.
What other mechanisms can we think we can utilize to prevent it?
- Checkpatch? Would be hard to write enough rules to find all possible patterns how creative people might use atomic as refcounter.
- People reviewing the code? Many kernel vulnerabilities live outside of core kernel, where maintainers are careful about what gets in and what's not. Further you go from core kernel (especially when you reach non-upstream drivers), code review quality is less, possibility of mistake is higher, and on average this code has more vulnerabilities. We can't say "this is not upstream code, who cares", because we want Linux kernel to follow "secure by default" principle: to provide enough mechanisms in kernel itself to minimize risk of mistakes and vulnerabilities. I think atomic is a great example of such case. We need to make it hard for people to make mistakes with overflows when overflows actually matter.
This was really a reason for our initial approach that provided "security by default". Certainly it had some issues (we all agree on this), but let's think how else can we provide "secure by default" protection for this.

2016-11-17 17:05:58

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Thu, Nov 17, 2016 at 01:01:49PM +0000, Reshetova, Elena wrote:

> >I think the sole motivator is a general distaste of atomic_t, which isn't a good reason at all.
>
> I don't think anyone has this as motivation. But atomic_t is so
> powerful and flexible that easily ends up being misused (as past CVEs
> shown).

I don't think using atomic_t as reference count is abuse. There simply
wasn't anything better. The proposed refcount_t cures this.

> Even if we now find all occurrences of atomic_t used as
> refcounter (which we cannot actually guarantee in any case unless
> someone manually reads every line) and convert it to refcount_t, we
> still have atomic_t type present and new usage of it as refount will
> crawl in. It is just a matter of time IMO.

Improve tooling. The patterns shouldn't be _that_ hard to find. Once the
tools are good, new code isn't a problem either.

Anything: atomic*_{{dec,sub}_and_test,{add,sub}_return,fetch_{add,sub}}
followed by a call_rcu()/free().

2016-11-17 17:07:53

[permalink] [raw]

Subject: RE: [RFC][PATCH 2/7] kref: Add kref_read()

> Even if we now find all occurrences of atomic_t used as refcounter
> (which we cannot actually guarantee in any case unless someone
> manually reads every line) and convert it to refcount_t, we still have
> atomic_t type present and new usage of it as refount will crawl in. It
> is just a matter of time IMO.

>Improve tooling. The patterns shouldn't be _that_ hard to find. Once the tools are good, new code isn't a problem either.

>Anything: atomic*_{{dec,sub}_and_test,{add,sub}_return,fetch_{add,sub}}
>followed by a call_rcu()/free().

Does not find everything unfortunately. Even if you add to above atomic*_add_unless() and also things like schedule_work(), still I fear we aren't covering everything.
What is worse, I don't think there is a mechanism to guarantee full coverage.

2016-11-17 17:19:33

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

2016-11-17 17:19:24

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Thu, 17 Nov 2016, Alexei Starovoitov wrote:
> On Thu, Nov 17, 2016 at 09:53:42AM +0100, Peter Zijlstra wrote:
> > On Wed, Nov 16, 2016 at 12:08:52PM -0800, Alexei Starovoitov wrote:
> >
> > > I prefer to avoid 'fixing' things that are not broken.
> > > Note, prog->aux->refcnt already has explicit checks for overflow.
> > > locked_vm is used for resource accounting and not refcnt,
> > > so I don't see issues there either.
> >
> > The idea is to use something along the lines of:
> >
> > http://lkml.kernel.org/r/[email protected]
> >
> > for all refcounts in the kernel.
>
> I understand the idea. I'm advocating to fix refcnts
> explicitly the way we did in bpf land instead of leaking memory,
> making processes unkillable and so on.
> If refcnt can be bounds checked, it should be done that way, since
> it's a clean error path without odd side effects.
> Therefore I'm against unconditionally applying refcount to all atomics.
>
> > Also note that your:
> >
> > struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
> > {
> > if (atomic_add_return(i, &prog->aux->refcnt) > BPF_MAX_REFCNT) {
> > atomic_sub(i, &prog->aux->refcnt);
> > return ERR_PTR(-EBUSY);
> > }
> > return prog;
> > }
> >
> > is actually broken in the face of an actual overflow. Suppose @i is big
> > enough to wrap refcnt into negative space.
>
> 'i' is not controlled by user. It's a number of nic hw queues
> and BPF_MAX_REFCNT is 32k, so above is always safe.
>
> > Also, the current sentiment is to strongly discourage add/sub operations
> > for refcounts.
>
> I agree with this reasoning as well, but it's not hard and fast rule.
> If we know we can do 'add' safely, we should.

In principle yes. OTOH, history shows that developers have a pretty bad
judgement what is safe and not. They rather copy code from random places,
modify it in creative ways and be done with it.

Thanks,

tglx

2016-11-17 17:27:50

by Alexei Starovoitov

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Thu, Nov 17, 2016 at 09:53:42AM +0100, Peter Zijlstra wrote:
> On Wed, Nov 16, 2016 at 12:08:52PM -0800, Alexei Starovoitov wrote:
>
> > I prefer to avoid 'fixing' things that are not broken.
> > Note, prog->aux->refcnt already has explicit checks for overflow.
> > locked_vm is used for resource accounting and not refcnt,
> > so I don't see issues there either.
>
> The idea is to use something along the lines of:
>
> http://lkml.kernel.org/r/[email protected]
>
> for all refcounts in the kernel.

I understand the idea. I'm advocating to fix refcnts
explicitly the way we did in bpf land instead of leaking memory,
making processes unkillable and so on.
If refcnt can be bounds checked, it should be done that way, since
it's a clean error path without odd side effects.
Therefore I'm against unconditionally applying refcount to all atomics.

> Also note that your:
>
> struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
> {
> if (atomic_add_return(i, &prog->aux->refcnt) > BPF_MAX_REFCNT) {
> atomic_sub(i, &prog->aux->refcnt);
> return ERR_PTR(-EBUSY);
> }
> return prog;
> }
>
> is actually broken in the face of an actual overflow. Suppose @i is big
> enough to wrap refcnt into negative space.

'i' is not controlled by user. It's a number of nic hw queues
and BPF_MAX_REFCNT is 32k, so above is always safe.

> Also, the current sentiment is to strongly discourage add/sub operations
> for refcounts.

I agree with this reasoning as well, but it's not hard and fast rule.
If we know we can do 'add' safely, we should.

2016-11-17 18:02:49

[permalink] [raw]

Subject: RE: [RFC][PATCH 2/7] kref: Add kref_read()

> Even if we now find all occurrences of atomic_t used as refcounter
> (which we cannot actually guarantee in any case unless someone
> manually reads every line) and convert it to refcount_t, we still have
> atomic_t type present and new usage of it as refount will crawl in. It
> is just a matter of time IMO.

>Improve tooling. The patterns shouldn't be _that_ hard to find. Once the tools are good, new code isn't a problem either.

Moreover, thinking of out of tree drivers: you think they would always do checkpatch or run some of our tools for security checks?

2016-11-17 19:10:39

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Thu, Nov 17, 2016 at 06:02:33PM +0000, Reshetova, Elena wrote:
>
> > Even if we now find all occurrences of atomic_t used as refcounter
> > (which we cannot actually guarantee in any case unless someone
> > manually reads every line) and convert it to refcount_t, we still have
> > atomic_t type present and new usage of it as refount will crawl in. It
> > is just a matter of time IMO.
>
> >Improve tooling. The patterns shouldn't be _that_ hard to find. Once the tools are good, new code isn't a problem either.
>
> Moreover, thinking of out of tree drivers: you think they would always
> do checkpatch or run some of our tools for security checks?

If they can't be arsed, neither can I. You can't fix the unfixable.

Like I said before, its chasing unicorns.

2016-11-17 19:30:11

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Thu, Nov 17, 2016 at 06:02:33PM +0000, Reshetova, Elena wrote:

> >Improve tooling. The patterns shouldn't be _that_ hard to find. Once the tools are good, new code isn't a problem either.
>
> Moreover, thinking of out of tree drivers: you think they would always
> do checkpatch or run some of our tools for security checks?

Also, checkpatch is a horrid example. That's mostly meaningless and
menial noise. Nobody wants to run that, even if, between all the
gibberish it lists a few sensible things.

Make an always enabled GCC plugin that generates build warns with a low
enough false positive rate and nobody will complain.

2016-11-17 19:34:53

by Kees Cook

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Thu, Nov 17, 2016 at 12:34 AM, Peter Zijlstra <[email protected]> wrote:
> On Wed, Nov 16, 2016 at 10:58:38AM -0800, Kees Cook wrote:
>> 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.

It's useful because its introduction creates a type that can't be
trivially used for refcounting (i.e. hard to make the mistake of using
stats_t for refcounting), and replacing atomic_t statistic counters
with stats_t reduces the effort required to do the initial (and
on-going) audit for misuse of atomic_t as a refcounter.

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

Your original suggestion about stats_t showed how its accessor API
would be a very small subset of the regular atomic_t set. I think that
reduction in accidental misuse has value.

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

I'd like to get to the point where auditing for mistakes in this area
is tractable. :) If atomic_t is only used for non-stats and
non-refcount, it's much much easier to examine and reason about.

-Kees

--
Kees Cook
Nexus Security

2016-11-18 17:34:02

[permalink] [raw]

Subject: RE: [RFC][PATCH 2/7] kref: Add kref_read()

On Thu, Nov 17, 2016 at 09:53:42AM +0100, Peter Zijlstra wrote:
> On Wed, Nov 16, 2016 at 12:08:52PM -0800, Alexei Starovoitov wrote:
>
> > I prefer to avoid 'fixing' things that are not broken.
> > Note, prog->aux->refcnt already has explicit checks for overflow.
> > locked_vm is used for resource accounting and not refcnt, so I don't
> > see issues there either.
>
> The idea is to use something along the lines of:
>
>
> http://lkml.kernel.org/r/[email protected]
> -ass.net
>
> for all refcounts in the kernel.

>I understand the idea. I'm advocating to fix refcnts explicitly the way we did in bpf land instead of leaking memory, making processes unkillable and so on.
>If refcnt can be bounds checked, it should be done that way, since it's a clean error path without odd side effects.
>Therefore I'm against unconditionally applying refcount to all atomics.

> Also note that your:
>
> struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i) {
> if (atomic_add_return(i, &prog->aux->refcnt) > BPF_MAX_REFCNT) {
> atomic_sub(i, &prog->aux->refcnt);
> return ERR_PTR(-EBUSY);
> }
> return prog;
> }
>
> is actually broken in the face of an actual overflow. Suppose @i is
> big enough to wrap refcnt into negative space.

>'i' is not controlled by user. It's a number of nic hw queues and BPF_MAX_REFCNT is 32k, so above is always safe.

If I understand your code right, you export the bpf_prog_add() and anyone is free to use it
(some crazy buggy driver for example).
Currently only drivers/net/ethernet/mellanox/mlx4/en_netdev.c uses it, but you should
consider any externally exposed interface as an attack vector from security point of view.
So, I would not claim that above construction is always safe since there is a way using API to
supply "i" that would overflow.

Next question is how to convert the above code sanely to refcount_t interface... Loop of inc(s)? Iikk...

2016-11-19 03:47:36

by Alexei Starovoitov

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Fri, Nov 18, 2016 at 05:33:35PM +0000, Reshetova, Elena wrote:
> On Thu, Nov 17, 2016 at 09:53:42AM +0100, Peter Zijlstra wrote:
> > On Wed, Nov 16, 2016 at 12:08:52PM -0800, Alexei Starovoitov wrote:
> >
> > > I prefer to avoid 'fixing' things that are not broken.
> > > Note, prog->aux->refcnt already has explicit checks for overflow.
> > > locked_vm is used for resource accounting and not refcnt, so I don't
> > > see issues there either.
> >
> > The idea is to use something along the lines of:
> >
> >
> > http://lkml.kernel.org/r/[email protected]
> > -ass.net
> >
> > for all refcounts in the kernel.
>
> >I understand the idea. I'm advocating to fix refcnts explicitly the way we did in bpf land instead of leaking memory, making processes unkillable and so on.
> >If refcnt can be bounds checked, it should be done that way, since it's a clean error path without odd side effects.
> >Therefore I'm against unconditionally applying refcount to all atomics.
>
> > Also note that your:
> >
> > struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i) {
> > if (atomic_add_return(i, &prog->aux->refcnt) > BPF_MAX_REFCNT) {
> > atomic_sub(i, &prog->aux->refcnt);
> > return ERR_PTR(-EBUSY);
> > }
> > return prog;
> > }
> >
> > is actually broken in the face of an actual overflow. Suppose @i is
> > big enough to wrap refcnt into negative space.
>
> >'i' is not controlled by user. It's a number of nic hw queues and BPF_MAX_REFCNT is 32k, so above is always safe.
>
> If I understand your code right, you export the bpf_prog_add() and anyone is free to use it
> (some crazy buggy driver for example).
> Currently only drivers/net/ethernet/mellanox/mlx4/en_netdev.c uses it, but you should
> consider any externally exposed interface as an attack vector from security point of view.

It's not realistic to harden all export_symbol apis.
Code review for in-tree modules is the only defense we have.
Remember out of tree perf counter issues... nothing perf core can do
about that. If it's out of tree, it's vendor's problem to fix it.

2016-11-21 08:18:16

[permalink] [raw]

Subject: RE: [RFC][PATCH 2/7] kref: Add kref_read()

> On Fri, Nov 18, 2016 at 05:33:35PM +0000, Reshetova, Elena wrote:
> > On Thu, Nov 17, 2016 at 09:53:42AM +0100, Peter Zijlstra wrote:
> > > On Wed, Nov 16, 2016 at 12:08:52PM -0800, Alexei Starovoitov wrote:
> > >
> > > > I prefer to avoid 'fixing' things that are not broken.
> > > > Note, prog->aux->refcnt already has explicit checks for overflow.
> > > > locked_vm is used for resource accounting and not refcnt, so I don't
> > > > see issues there either.
> > >
> > > The idea is to use something along the lines of:
> > >
> > >
> > >
> http://lkml.kernel.org/r/[email protected]
> > > -ass.net
> > >
> > > for all refcounts in the kernel.
> >
> > >I understand the idea. I'm advocating to fix refcnts explicitly the way we did
> in bpf land instead of leaking memory, making processes unkillable and so on.
> > >If refcnt can be bounds checked, it should be done that way, since it's a
> clean error path without odd side effects.
> > >Therefore I'm against unconditionally applying refcount to all atomics.
> >
> > > Also note that your:
> > >
> > > struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i) {
> > > if (atomic_add_return(i, &prog->aux->refcnt) > BPF_MAX_REFCNT) {
> > > atomic_sub(i, &prog->aux->refcnt);
> > > return ERR_PTR(-EBUSY);
> > > }
> > > return prog;
> > > }
> > >
> > > is actually broken in the face of an actual overflow. Suppose @i is
> > > big enough to wrap refcnt into negative space.
> >
> > >'i' is not controlled by user. It's a number of nic hw queues and
> BPF_MAX_REFCNT is 32k, so above is always safe.
> >
> > If I understand your code right, you export the bpf_prog_add() and anyone is
> free to use it
> > (some crazy buggy driver for example).
> > Currently only drivers/net/ethernet/mellanox/mlx4/en_netdev.c uses it, but
> you should
> > consider any externally exposed interface as an attack vector from security
> point of view.
>
> It's not realistic to harden all export_symbol apis.
> Code review for in-tree modules is the only defense we have.
> Remember out of tree perf counter issues... nothing perf core can do
> about that. If it's out of tree, it's vendor's problem to fix it.

I am not trying to harden them all now, but since we are going through the list of
atomic_t variables that are used for refcounting, and this seems to be the one,
I was trying to find a way to convert it also since it isn't a big effort and would do
good at the end.
So, you are not fine if I convert the above code using only refcount_inc and refcount_dec_and_test
primitives?

2016-11-21 12:47:15

by David Windsor

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Fri, Nov 18, 2016 at 12:33 PM, Reshetova, Elena
<[email protected]> wrote:
> On Thu, Nov 17, 2016 at 09:53:42AM +0100, Peter Zijlstra wrote:
>> On Wed, Nov 16, 2016 at 12:08:52PM -0800, Alexei Starovoitov wrote:
>>
>> > I prefer to avoid 'fixing' things that are not broken.
>> > Note, prog->aux->refcnt already has explicit checks for overflow.
>> > locked_vm is used for resource accounting and not refcnt, so I don't
>> > see issues there either.
>>
>> The idea is to use something along the lines of:
>>
>>
>> http://lkml.kernel.org/r/[email protected]
>> -ass.net
>>
>> for all refcounts in the kernel.
>
>>I understand the idea. I'm advocating to fix refcnts explicitly the way we did in bpf land instead of leaking memory, making processes unkillable and so on.
>>If refcnt can be bounds checked, it should be done that way, since it's a clean error path without odd side effects.
>>Therefore I'm against unconditionally applying refcount to all atomics.
>
>> Also note that your:
>>
>> struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i) {
>> if (atomic_add_return(i, &prog->aux->refcnt) > BPF_MAX_REFCNT) {
>> atomic_sub(i, &prog->aux->refcnt);
>> return ERR_PTR(-EBUSY);
>> }
>> return prog;
>> }
>>
>> is actually broken in the face of an actual overflow. Suppose @i is
>> big enough to wrap refcnt into negative space.
>
>>'i' is not controlled by user. It's a number of nic hw queues and BPF_MAX_REFCNT is 32k, so above is always safe.
>
> If I understand your code right, you export the bpf_prog_add() and anyone is free to use it
> (some crazy buggy driver for example).
> Currently only drivers/net/ethernet/mellanox/mlx4/en_netdev.c uses it, but you should
> consider any externally exposed interface as an attack vector from security point of view.
> So, I would not claim that above construction is always safe since there is a way using API to
> supply "i" that would overflow.
>
> Next question is how to convert the above code sanely to refcount_t interface... Loop of inc(s)? Iikk...
>

By the way, there are several sites where the use of
atomic_t/atomic_wrap_t as a counter ventures beyond the standard (inc,
dec, add, sub, read, set) operations we're planning on implementing
for both refcount_t and stats_t. While performing the conversion to
stats_t, I've found usage of atomic_xchg(), for instance. From
kernel/trace/trace_mmiotrace.c:123:

unsigned long cnt = atomic_xchg(&dropped_count, 0);

stats_xchg() isn't anticipated to go into the stats_t API, and
dropped_count clearly appears to be a statistical counter, so we will
have to further audit this site to determine whether the atomicity of
the atomic_xchg() operation is truly necessary here. If it is, we
can either decide to implement stats_xchg(), or we could use a
combination of locking, stats_read() and stats_set() to accomplish the
same thing as stats_xchg().

>
>

2016-11-21 15:39:24

[permalink] [raw]

Subject: RE: [RFC][PATCH 2/7] kref: Add kref_read()

> By the way, there are several sites where the use of
> atomic_t/atomic_wrap_t as a counter ventures beyond the standard (inc,
> dec, add, sub, read, set) operations we're planning on implementing
> for both refcount_t and stats_t.

Speaking of non-fitting patterns. This one is quite common in networking code for refcounters:

if (atomic_cmpxchg(&cur->refcnt, 1, 0) == 1) {}
This is from net/netfilter/nfnetlink_acct.c, but there are similar ones in other places.

Also, simple atomic_dec() is used pretty much everywhere for counters, which we don’t have a straight match in refcount_t API.

2016-11-21 15:49:35

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Mon, Nov 21, 2016 at 03:39:19PM +0000, Reshetova, Elena wrote:
> > By the way, there are several sites where the use of
> > atomic_t/atomic_wrap_t as a counter ventures beyond the standard (inc,
> > dec, add, sub, read, set) operations we're planning on implementing
> > for both refcount_t and stats_t.
>
> Speaking of non-fitting patterns. This one is quite common in
> networking code for refcounters:
>
> if (atomic_cmpxchg(&cur->refcnt, 1, 0) == 1) {} This is from
> net/netfilter/nfnetlink_acct.c, but there are similar ones in other
> places.

Cute, but weird it doesn't actually decrement if not 1.

> Also, simple atomic_dec() is used pretty much everywhere for counters,
> which we don’t have a straight match in refcount_t API.

WARN_ON(refcount_dec_and_test(refs));

And seeing how I've implememented refcount_inc() in similar terms:

WARN_ON(!refcount_inc_not_zero(refs));

It might make sense to actually provide that.

2016-11-21 16:01:16

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Mon, Nov 21, 2016 at 04:49:15PM +0100, Peter Zijlstra wrote:
> > Speaking of non-fitting patterns. This one is quite common in
> > networking code for refcounters:
> >
> > if (atomic_cmpxchg(&cur->refcnt, 1, 0) == 1) {} This is from
> > net/netfilter/nfnetlink_acct.c, but there are similar ones in other
> > places.
>
> Cute, but weird it doesn't actually decrement if not 1.

Hurgh.. creative refcounting that. The question is how much of that do
we want to support? It really must not decrement there.

2016-11-21 19:27:45

[permalink] [raw]

Subject: RE: [RFC][PATCH 2/7] kref: Add kref_read()

> On Mon, Nov 21, 2016 at 04:49:15PM +0100, Peter Zijlstra wrote:
> > > Speaking of non-fitting patterns. This one is quite common in
> > > networking code for refcounters:
> > >
> > > if (atomic_cmpxchg(&cur->refcnt, 1, 0) == 1) {} This is from
> > > net/netfilter/nfnetlink_acct.c, but there are similar ones in other
> > > places.
> >
> > Cute, but weird it doesn't actually decrement if not 1.
>
> Hurgh.. creative refcounting that. The question is how much of that do
> we want to support? It really must not decrement there.

And one more creative usage:

http://lxr.free-electrons.com/source/net/ipv4/udp.c#L1940

if (!sk || !atomic_inc_not_zero_hint(&sk->sk_refcnt, 2))
return;

I didn't even guess anyone is using atomic_inc_not_zero_hint...
But network code keeps surprising me today :)
So, yes, I guess the question is what to do with these cases really?

2016-11-21 20:12:36

by David Windsor

[permalink] [raw]

Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read()

On Mon, Nov 21, 2016 at 2:27 PM, Reshetova, Elena
<[email protected]> wrote:
>> On Mon, Nov 21, 2016 at 04:49:15PM +0100, Peter Zijlstra wrote:
>> > > Speaking of non-fitting patterns. This one is quite common in
>> > > networking code for refcounters:
>> > >
>> > > if (atomic_cmpxchg(&cur->refcnt, 1, 0) == 1) {} This is from
>> > > net/netfilter/nfnetlink_acct.c, but there are similar ones in other
>> > > places.
>> >
>> > Cute, but weird it doesn't actually decrement if not 1.
>>
>> Hurgh.. creative refcounting that. The question is how much of that do
>> we want to support? It really must not decrement there.
>
> And one more creative usage:
>
> http://lxr.free-electrons.com/source/net/ipv4/udp.c#L1940
>
> if (!sk || !atomic_inc_not_zero_hint(&sk->sk_refcnt, 2))
> return;
>
> I didn't even guess anyone is using atomic_inc_not_zero_hint...
> But network code keeps surprising me today :)
> So, yes, I guess the question is what to do with these cases really?

Many of the calls to non-supported functions can be decomposed into
calls to supported functions. The ones that may prove interesting are
ones like atomic_cmpxchg(), in which some sort of external locking is
going to be required to achieve the same atomicity guarantees provided
by cmpxchg, like so:

mutex_lock(lock);
cnt = refcount_read(ref);
if (cnt == val1) {
refcount_set(ref, val2);
}
mutex_unlock(lock);
return cnt;

2016-11-22 10:37:39