RE: [RFCv2 4/4] bpf: inhibit speculated out-of-bounds pointers

[Reshetova, Elena]

> On Fri, Jan 05, 2018 at 02:57:50PM +, Mark Rutland wrote:
> > Note: this patch is an *example* use of the nospec API. It is understood
> > that this is incomplete, etc.
> >
> > Under speculation, CPUs may mis-predict branches in bounds checks. Thus,
> > memory accesses under a bounds check may be speculated even if the
> > bounds check fails, providing a primitive for building a side channel.
> >
> > The EBPF map code has a number of such bounds-checks accesses in
> > map_lookup_elem implementations. This patch modifies these to use the
> > nospec helpers to inhibit such side channels.
> >
> > The JITted lookup_elem implementations remain potentially vulnerable,
> > and are disabled (with JITted code falling back to the C
> > implementations).
> >
> > Signed-off-by: Mark Rutland 
> > Signed-off-by: Will Deacon 
> > Cc: Dan Williams 
> > Cc: Peter Zijlstra 
> > ---
> >  kernel/bpf/arraymap.c | 20 +---
> >  kernel/bpf/cpumap.c   |  5 ++---
> >  kernel/bpf/devmap.c   |  3 ++-
> >  kernel/bpf/sockmap.c  |  3 ++-
> >  4 files changed, 19 insertions(+), 12 deletions(-)
> 
> Mark, did you see my email with this patch yesterday ?
> https://patchwork.ozlabs.org/patch/855911/
> 
> btw your patch does not fix the variant 1 exploit.
> 
> Also all of the pre-embargo patches from Elena that add lfence
> in the bpf interpreter and x64 JIT also do not fix it.
> 
> The exploit works via bpf_tail_call and not via map_lookup.

Could you please clarify this part? The actual jump
to the out-of-bounds index is indeed made by bpf_tail_call, 
but the "speculation" bypassing step happens when it does map_lookup_elem
on the out-of-bound index.  


Best Regards,
Elena.

RE: [RFC PATCH] asm/generic: introduce if_nospec and nospec_barrier

[Reshetova, Elena]

> On Thu, Jan 04, 2018 at 02:15:53AM +, Alan Cox wrote:
> >
> > > > Elena has done the work of auditing static analysis reports to a dozen
> > > > or so locations that need some 'nospec' handling.
> > >
> > > How exactly is that related (especially in longer-term support terms) to
> > > BPF anyway?
> >
> > If you read the papers you need a very specific construct in order to not
> > only cause a speculative load of an address you choose but also to then
> > manage to cause a second operation that in some way reveals bits of data
> > or allows you to ask questions.
> >
> > BPF allows you to construct those sequences relatively easily and it's
> > the one case where a user space application can fairly easily place code
> > it wants to execute in the kernel. Without BPF you have to find the right
> > construct in the kernel, prime all the right predictions and measure the
> > result without getting killed off. There are places you can do that but
> > they are not so easy and we don't (at this point) think there are that
> > many.
> 
> for BPF in particular we're thinking to do a different fix.
> Instead of killing speculation we can let cpu speculate.
> The fix will include rounding up bpf maps to nearest power of 2 and
> inserting bpf_and operation on the index after bounds check,
> so cpu can continue speculate beyond bounds check, but it will
> load from zero-ed memory.
> So this nospec arch dependent hack won't be necessary for bpf side,
> but may still be needed in other parts of the kernel.

I think this is a much better approach than what we have been doing internally 
so
far. My initial fix back in August for this was to insert a minimal set of 
lfence
barriers (osb() barrier below) that prevent speculation just based on how 
attack was using constants
to index eBPF maps:

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
#define BPF_R0  regs[BPF_REG_0]
@@ -939,6 +940,7 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct 
bpf_insn *insn,
DST = IMM;
CONT;
LD_IMM_DW:
+   osb();
DST = (u64) (u32) insn[0].imm | ((u64) (u32) insn[1].imm) << 32;
insn++;
CONT;
@@ -1200,6 +1202,7 @@ static unsigned int ___bpf_prog_run(u64 *regs, const 
struct bpf_insn *insn,
*(SIZE *)(unsigned long) (DST + insn->off) = IMM;   \
CONT;   \
LDX_MEM_##SIZEOP:   \
+   osb();  \
DST = *(SIZE *)(unsigned long) (SRC + insn->off);   \
CONT;
 


And similar stuff also for x86 JIT (blinding dependent):

@@ -400,7 +413,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 
*image,
case BPF_ADD: b2 = 0x01; break;
case BPF_SUB: b2 = 0x29; break;
case BPF_AND: b2 = 0x21; break;
-   case BPF_OR: b2 = 0x09; break;
+   case BPF_OR: b2 = 0x09; emit_memory_barrier(); 
break;
case BPF_XOR: b2 = 0x31; break;
}
if (BPF_CLASS(insn->code) == BPF_ALU64)
@@ -647,6 +660,16 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, 
u8 *image,
case BPF_ALU64 | BPF_RSH | BPF_X:
case BPF_ALU64 | BPF_ARSH | BPF_X:
 
+   /* If blinding is enabled, each
+* BPF_LD | BPF_IMM | BPF_DW instruction
+* is converted to 4 eBPF instructions with
+* BPF_ALU64_IMM(BPF_LSH, BPF_REG_AX, 32)
+* always present(number 3). Detect such cases
+* and insert memory barriers. */
+   if ((BPF_CLASS(insn->code) == BPF_ALU64)
+   && (BPF_OP(insn->code) == BPF_LSH)
+   && (src_reg == BPF_REG_AX))
+   emit_memory_barrier();
/* check for bad case when dst_reg == rcx */
if (dst_reg == BPF_REG_4) {
/* mov r11, dst_reg */

But this is really ugly fix IMO to prevent speculation from happen, so with your
approach I think it is really much better. 

If you need help in testing the fixes, I can do it unless you already have the
attack code yourself. 

> 
> Also note that variant 1 is talking about exploiting prog_array
> bpf feature which had 64-bit access prior to
> commit 90caccdd8cc0 ("bpf: fix bpf_tail_call() x64 JIT")
> That was a fix for JIT and not related to this cpu issue, but
> I believe it breaks the existing exploit.

Actually I could not get existing exploit working on anything past 4.11
but at that time I could not see any fundamental change in eBPF that
would prevent it. 

Best Regards,
Elena.

RE: [PATCH 02/15] drivers, net, ethernet: convert mtk_eth.dma_refcnt from atomic_t to refcount_t

[Reshetova, Elena]

> On Fri, 2017-10-20 at 10:37 +0000, Reshetova, Elena wrote:
> > > On Fri, 2017-10-20 at 10:23 +0300, Elena Reshetova wrote:
> > > > atomic_t variables are currently used to implement reference
> > > > counters with the following properties:
> > > >  - counter is initialized to 1 using atomic_set()
> > > >  - a resource is freed upon counter reaching zero
> > > >  - once counter reaches zero, its further
> > > >increments aren't allowed
> > > >  - counter schema uses basic atomic operations
> > > >(set, inc, inc_not_zero, dec_and_test, etc.)
> > > >
> > > > Such atomic variables should be converted to a newly provided
> > > > refcount_t type and API that prevents accidental counter overflows
> > > > and underflows. This is important since overflows and underflows
> > > > can lead to use-after-free situation and be exploitable.
> > > >
> > > > The variable mtk_eth.dma_refcnt is used as pure reference counter.
> > > > Convert it to refcount_t and fix up the operations.
> > > >
> > > > Suggested-by: Kees Cook <keesc...@chromium.org>
> > > > Reviewed-by: David Windsor <dwind...@gmail.com>
> > > > Reviewed-by: Hans Liljestrand <ishkam...@gmail.com>
> > > > Signed-off-by: Elena Reshetova <elena.reshet...@intel.com>
> > > > ---
> > > >  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 8 +---
> > > >  drivers/net/ethernet/mediatek/mtk_eth_soc.h | 4 +++-
> > > >  2 files changed, 8 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > > index 5e81a72..54adfd9 100644
> > > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > > > @@ -1817,7 +1817,7 @@ static int mtk_open(struct net_device *dev)
> > > > struct mtk_eth *eth = mac->hw;
> > > >
> > > > /* we run 2 netdevs on the same dma ring so we only bring it up 
> > > > once
> > > */
> > > > -   if (!atomic_read(>dma_refcnt)) {
> > > > +   if (!refcount_read(>dma_refcnt)) {
> > > > int err = mtk_start_dma(eth);
> > > >
> > > > if (err)
> > > > @@ -1827,8 +1827,10 @@ static int mtk_open(struct net_device *dev)
> > > > napi_enable(>rx_napi);
> > > > mtk_tx_irq_enable(eth, MTK_TX_DONE_INT);
> > > > mtk_rx_irq_enable(eth, MTK_RX_DONE_INT);
> > > > +   refcount_set(>dma_refcnt, 1);
> > >
> > > the existing driver seems to have a missing initial atomic_set for the
> > > eth->dma_refcnt.
> > >
> > > > }
> > > > -   atomic_inc(>dma_refcnt);
> > > > +   else
> > > > +   refcount_inc(>dma_refcnt);
> > > >
> > >
> > > how about add the initial refcount_set into probe handler, and keep
> > > logic else unchanged ?
> >
> > Sure, I guess you mean mtk_probe() function? I can move the refcount_set to 
> > be
> there
> > and remove this change.
> >
> > Should I resend the modified patch to you (maybe then two of the ethernet
> patches)?
> >
> > Best Regards,
> > Elena.
> 
> The entire series has been applies to net-next, I think I can make the
> follow-ups patches relative to your work.
> 
>   Sean

Yes, I just noticed that David took them all. 
Sure, if you want to send the follow up yourself, I certainly would not mind, 
I still have many of these recount patches for different parts of kernel :)

Thank you!

Best Regards,
Elena.
> 
> > >
> > > > phy_start(dev->phydev);
> > > > netif_start_queue(dev);
> > > > @@ -1868,7 +1870,7 @@ static int mtk_stop(struct net_device *dev)
> > > > phy_stop(dev->phydev);
> > > >
> > > > /* only shutdown DMA if this is the last user */
> > > > -   if (!atomic_dec_and_test(>dma_refcnt))
> > > > +   if (!refcount_dec_and_test(>dma_refcnt))
> > > > return 0;
> > > >
> > > > mtk_tx_irq_disable(eth, MTK_TX_DONE_INT);
> > > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> > > b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> > > > index 3d3c24a..a3af466 100644
> > > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> > > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> > > > @@ -15,6 +15,8 @@
> > > >  #ifndef MTK_ETH_H
> > > >  #define MTK_ETH_H
> > > >
> > > > +#include 
> > > > +
> > > >  #define MTK_QDMA_PAGE_SIZE 2048
> > > >  #defineMTK_MAX_RX_LENGTH   1536
> > > >  #define MTK_TX_DMA_BUF_LEN 0x3fff
> > > > @@ -632,7 +634,7 @@ struct mtk_eth {
> > > > struct regmap   *pctl;
> > > > u32 chip_id;
> > > > boolhwlro;
> > > > -   atomic_tdma_refcnt;
> > > > +   refcount_t  dma_refcnt;
> > > > struct mtk_tx_ring  tx_ring;
> > > > struct mtk_rx_ring
> > >   rx_ring[MTK_MAX_RX_RING_NUM];
> > > > struct mtk_rx_ring  rx_ring_qdma;
> > >
> >
> 

RE: [PATCH 00/15] networking drivers refcount_t conversions

[Reshetova, Elena]

> From: Elena Reshetova 
> Date: Fri, 20 Oct 2017 10:23:34 +0300
> 
> > Note: these are the last patches related to networking that perform
> > conversion of refcounters from atomic_t to refcount_t.
> > In contrast to the core network refcounter conversions that
> > were merged earlier, these are much more straightforward ones.
> >
> > This series, for various networking drivers, replaces atomic_t reference
> > counters with the new refcount_t type and API (see 
> > include/linux/refcount.h).
> > By doing this we prevent intentional or accidental
> > underflows or overflows that can led to use-after-free vulnerabilities.
> >
> > The patches are fully independent and can be cherry-picked separately.
> > Patches are based on top of net-next.
> > If there are no objections to the patches, please merge them via respective 
> > trees
> 
> I've applied this entire series to net-next.  If there are any fixups or
> follow-ups please send them as relative patches.
> 
> Thank you.

Thank you very much David! Will send fixups separately.

Best Regards,
Elena.

RE: [PATCH 02/15] drivers, net, ethernet: convert mtk_eth.dma_refcnt from atomic_t to refcount_t

[Reshetova, Elena]

> On Fri, 2017-10-20 at 10:23 +0300, Elena Reshetova wrote:
> > atomic_t variables are currently used to implement reference
> > counters with the following properties:
> >  - counter is initialized to 1 using atomic_set()
> >  - a resource is freed upon counter reaching zero
> >  - once counter reaches zero, its further
> >increments aren't allowed
> >  - counter schema uses basic atomic operations
> >(set, inc, inc_not_zero, dec_and_test, etc.)
> >
> > Such atomic variables should be converted to a newly provided
> > refcount_t type and API that prevents accidental counter overflows
> > and underflows. This is important since overflows and underflows
> > can lead to use-after-free situation and be exploitable.
> >
> > The variable mtk_eth.dma_refcnt is used as pure reference counter.
> > Convert it to refcount_t and fix up the operations.
> >
> > Suggested-by: Kees Cook 
> > Reviewed-by: David Windsor 
> > Reviewed-by: Hans Liljestrand 
> > Signed-off-by: Elena Reshetova 
> > ---
> >  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 8 +---
> >  drivers/net/ethernet/mediatek/mtk_eth_soc.h | 4 +++-
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > index 5e81a72..54adfd9 100644
> > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> > @@ -1817,7 +1817,7 @@ static int mtk_open(struct net_device *dev)
> > struct mtk_eth *eth = mac->hw;
> >
> > /* we run 2 netdevs on the same dma ring so we only bring it up once
> */
> > -   if (!atomic_read(>dma_refcnt)) {
> > +   if (!refcount_read(>dma_refcnt)) {
> > int err = mtk_start_dma(eth);
> >
> > if (err)
> > @@ -1827,8 +1827,10 @@ static int mtk_open(struct net_device *dev)
> > napi_enable(>rx_napi);
> > mtk_tx_irq_enable(eth, MTK_TX_DONE_INT);
> > mtk_rx_irq_enable(eth, MTK_RX_DONE_INT);
> > +   refcount_set(>dma_refcnt, 1);
> 
> the existing driver seems to have a missing initial atomic_set for the
> eth->dma_refcnt.
> 
> > }
> > -   atomic_inc(>dma_refcnt);
> > +   else
> > +   refcount_inc(>dma_refcnt);
> >
> 
> how about add the initial refcount_set into probe handler, and keep
> logic else unchanged ?

Sure, I guess you mean mtk_probe() function? I can move the refcount_set to be 
there
and remove this change. 

Should I resend the modified patch to you (maybe then two of the ethernet 
patches)?

Best Regards,
Elena.
> 
> > phy_start(dev->phydev);
> > netif_start_queue(dev);
> > @@ -1868,7 +1870,7 @@ static int mtk_stop(struct net_device *dev)
> > phy_stop(dev->phydev);
> >
> > /* only shutdown DMA if this is the last user */
> > -   if (!atomic_dec_and_test(>dma_refcnt))
> > +   if (!refcount_dec_and_test(>dma_refcnt))
> > return 0;
> >
> > mtk_tx_irq_disable(eth, MTK_TX_DONE_INT);
> > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> > index 3d3c24a..a3af466 100644
> > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> > @@ -15,6 +15,8 @@
> >  #ifndef MTK_ETH_H
> >  #define MTK_ETH_H
> >
> > +#include 
> > +
> >  #define MTK_QDMA_PAGE_SIZE 2048
> >  #defineMTK_MAX_RX_LENGTH   1536
> >  #define MTK_TX_DMA_BUF_LEN 0x3fff
> > @@ -632,7 +634,7 @@ struct mtk_eth {
> > struct regmap   *pctl;
> > u32 chip_id;
> > boolhwlro;
> > -   atomic_tdma_refcnt;
> > +   refcount_t  dma_refcnt;
> > struct mtk_tx_ring  tx_ring;
> > struct mtk_rx_ring
>   rx_ring[MTK_MAX_RX_RING_NUM];
> > struct mtk_rx_ring  rx_ring_qdma;
> 

RE: [PATCH 00/17] v3 net generic subsystem refcount conversions

[Reshetova, Elena]

> On Mon, Jul 03, 2017 at 02:28:56AM -0700, Eric Dumazet wrote:
> >On Fri, 2017-06-30 at 13:07 +0300, Elena Reshetova wrote:
> >> Changes in v3:
> >> Rebased on top of the net-next tree.
> >>
> >> Changes in v2:
> >> No changes in patches apart from rebases, but now by
> >> default refcount_t = atomic_t (*) and uses all atomic standard operations
> >> unless CONFIG_REFCOUNT_FULL is enabled. This is a compromise for the
> >> systems that are critical on performance (such as net) and cannot accept 
> >> even
> >> slight delay on the refcounter operations.
> >>
> >> This series, for core network subsystem components, replaces atomic_t
> reference
> >> counters with the new refcount_t type and API (see
> include/linux/refcount.h).
> >> By doing this we prevent intentional or accidental
> >> underflows or overflows that can led to use-after-free vulnerabilities.
> >> These patches contain only generic net pieces. Other changes will be sent
> separately.
> >>
> >> The patches are fully independent and can be cherry-picked separately.
> >> The big patches, such as conversions for sock structure, need a very 
> >> detailed
> >> look from maintainers: refcount managing is quite complex in them and while
> >> it seems that they would benefit from the change, extra checking is needed.
> >> The biggest corner issue is the fact that refcount_inc() does not increment
> >> from zero.
> >>
> >> If there are no objections to the patches, please merge them via respective
> trees.
> >>
> >> * The respective change is currently merged into -next as
> >>   "locking/refcount: Create unchecked atomic_t implementation".
> >>
> >> Elena Reshetova (17):
> >>   net: convert inet_peer.refcnt from atomic_t to refcount_t
> >>   net: convert neighbour.refcnt from atomic_t to refcount_t
> >>   net: convert neigh_params.refcnt from atomic_t to refcount_t
> >>   net: convert nf_bridge_info.use from atomic_t to refcount_t
> >>   net: convert sk_buff.users from atomic_t to refcount_t
> >>   net: convert sk_buff_fclones.fclone_ref from atomic_t to refcount_t
> >>   net: convert sock.sk_wmem_alloc from atomic_t to refcount_t
> >>   net: convert sock.sk_refcnt from atomic_t to refcount_t
> >>   net: convert ip_mc_list.refcnt from atomic_t to refcount_t
> >>   net: convert in_device.refcnt from atomic_t to refcount_t
> >>   net: convert netpoll_info.refcnt from atomic_t to refcount_t
> >>   net: convert unix_address.refcnt from atomic_t to refcount_t
> >>   net: convert fib_rule.refcnt from atomic_t to refcount_t
> >>   net: convert inet_frag_queue.refcnt from atomic_t to refcount_t
> >>   net: convert net.passive from atomic_t to refcount_t
> >>   net: convert netlbl_lsm_cache.refcount from atomic_t to refcount_t
> >>   net: convert packet_fanout.sk_ref from atomic_t to refcount_t
> >
> >
> >Can you take a look at this please ?
> >
> >[   64.601749] [ cut here ]
> >[   64.601757] WARNING: CPU: 0 PID: 6476 at lib/refcount.c:184
> refcount_sub_and_test+0x75/0xa0
> >[   64.601758] Modules linked in: w1_therm wire cdc_acm ehci_pci ehci_hcd
> mlx4_en ib_uverbs mlx4_ib ib_core mlx4_core
> >[   64.601769] CPU: 0 PID: 6476 Comm: ip Tainted: GW   
> >4.12.0-smp-DEV
> #274
> >[   64.601770] Hardware name: Intel RML,PCH/Iota_QC_19, BIOS 2.40.0
> 06/22/2016
> >[   64.601771] task: 8837bf482040 task.stack: 8837bdc08000
> >[   64.601773] RIP: 0010:refcount_sub_and_test+0x75/0xa0
> >[   64.601774] RSP: 0018:8837bdc0f5c0 EFLAGS: 00010286
> >[   64.601776] RAX: 0026 RBX: 0001 RCX:
> 
> >[   64.601777] RDX: 0026 RSI: 0096 RDI:
> ed06f7b81eae
> >[   64.601778] RBP: 8837bdc0f5d0 R08: 0004 R09:
> fbfff4a54c25
> >[   64.601779] R10: cbc500e5 R11: a52a6128 R12: 
> >881febcf6f24
> >[   64.601779] R13: 881fbf4eaf00 R14: 881febcf6f80 R15: 
> >8837d7a4ed00
> >[   64.601781] FS:  7ff5a2f6b700() GS:881fff80()
> knlGS:
> >[   64.601782] CS:  0010 DS:  ES:  CR0: 80050033
> >[   64.601783] CR2: 7ffcdc70d000 CR3: 001f9c91e000 CR4:
> 001406f0
> >[   64.601783] Call Trace:
> >[   64.601786]  refcount_dec_and_test+0x11/0x20
> >[   64.601790]  fib_nl_delrule+0xc39/0x1630
> [snip]
> 
> I'm seeing a similar one coming from sctp:
> 
> refcount_t: underflow; use-after-free.
> [ cut here ]
> WARNING: CPU: 3 PID: 15570 at lib/refcount.c:186
> refcount_sub_and_test.cold.13+0x18/0x21 lib/refcount.c:186
> Kernel panic - not syncing: panic_on_warn set ...
> 
> CPU: 3 PID: 15570 Comm: syz-executor0 Not tainted 4.12.0-next-20170706+ #186
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.1-1ubuntu1
> 04/01/2014
> Call Trace:
>  __dump_stack lib/dump_stack.c:16 [inline]
>  dump_stack+0x11d/0x1ef lib/dump_stack.c:52
>  panic+0x1bc/0x3ad kernel/panic.c:180
>  __warn.cold.6+0x2f/0x2f kernel/panic.c:541
>  

RE: [PATCH 00/36] v2 net subsystem misc refcounter conversions

[Reshetova, Elena]

> From: Elena Reshetova 
> Date: Tue,  4 Jul 2017 15:52:55 +0300
> 
> > Changes in v2:
> >  * rebase on top of net-next
> >  * currently by default refcount_t = atomic_t (*) and uses all
> >atomic standard operations unless CONFIG_REFCOUNT_FULL is enabled.
> >This is a compromise for the systems that are critical on
> >performance (such as net) and cannot accept even slight delay
> >on the refcounter operations.
> >
> > This series, for various misc network components, replaces atomic_t 
> > reference
> > counters with the new refcount_t type and API (see 
> > include/linux/refcount.h).
> > By doing this we prevent intentional or accidental
> > underflows or overflows that can led to use-after-free vulnerabilities.
> > These are the last networking-related conversions with the exception of
> > network drivers (to be send separately).
> >
> > Please excuse the long patch set, but seems like breaking it up
> > won't save that much on CC list and most of the changes are
> > trivial.
> >
> > The patches are fully independent and can be cherry-picked separately.
> > In order to try with refcount functionality enabled in run-time,
> > CONFIG_REFCOUNT_FULL must be enabled.
> >
> > NOTE: automatic kernel builder for some reason doesn't like all my
> > network branches and regularly times out the builds on these branches.
> > Suggestion for "waiting a day for a good coverage" doesn't work, as
> > we have seen with generic network conversions. So please wait for the
> > full report from kernel test rebot before merging further up.
> > This has been compile-tested in 116 configs, but 71 timed out (including
> > all s390-related configs again). I am trying to see if they can fix
> > build coverage for me in meanwhile.
> >
> > * The respective change is currently merged into -next as
> >   "locking/refcount: Create unchecked atomic_t implementation".
> 
> Series applied, that's enough for this cycle, please.

Thank you very much David! I really appreciate you taking so many of these 
conversions in
one go! I will stop for now :) 

With regards to net, I only have networking drivers left (16 patches in total), 
but I can submit them in the
next round. Will go bug other subsystem maintainers next :) 

Best Regards,
Elena.

RE: [PATCH 00/17] v3 net generic subsystem refcount conversions

[Reshetova, Elena]

> On Fri, 2017-06-30 at 13:07 +0300, Elena Reshetova wrote:
> > Changes in v3:
> > Rebased on top of the net-next tree.
> >
> > Changes in v2:
> > No changes in patches apart from rebases, but now by
> > default refcount_t = atomic_t (*) and uses all atomic standard operations
> > unless CONFIG_REFCOUNT_FULL is enabled. This is a compromise for the
> > systems that are critical on performance (such as net) and cannot accept 
> > even
> > slight delay on the refcounter operations.
> >
> > This series, for core network subsystem components, replaces atomic_t 
> > reference
> > counters with the new refcount_t type and API (see 
> > include/linux/refcount.h).
> > By doing this we prevent intentional or accidental
> > underflows or overflows that can led to use-after-free vulnerabilities.
> > These patches contain only generic net pieces. Other changes will be sent
> separately.
> >
> > The patches are fully independent and can be cherry-picked separately.
> > The big patches, such as conversions for sock structure, need a very 
> > detailed
> > look from maintainers: refcount managing is quite complex in them and while
> > it seems that they would benefit from the change, extra checking is needed.
> > The biggest corner issue is the fact that refcount_inc() does not increment
> > from zero.
> >
> > If there are no objections to the patches, please merge them via respective 
> > trees.
> >
> > * The respective change is currently merged into -next as
> >   "locking/refcount: Create unchecked atomic_t implementation".
> >
> > Elena Reshetova (17):
> >   net: convert inet_peer.refcnt from atomic_t to refcount_t
> >   net: convert neighbour.refcnt from atomic_t to refcount_t
> >   net: convert neigh_params.refcnt from atomic_t to refcount_t
> >   net: convert nf_bridge_info.use from atomic_t to refcount_t
> >   net: convert sk_buff.users from atomic_t to refcount_t
> >   net: convert sk_buff_fclones.fclone_ref from atomic_t to refcount_t
> >   net: convert sock.sk_wmem_alloc from atomic_t to refcount_t
> >   net: convert sock.sk_refcnt from atomic_t to refcount_t
> >   net: convert ip_mc_list.refcnt from atomic_t to refcount_t
> >   net: convert in_device.refcnt from atomic_t to refcount_t
> >   net: convert netpoll_info.refcnt from atomic_t to refcount_t
> >   net: convert unix_address.refcnt from atomic_t to refcount_t
> >   net: convert fib_rule.refcnt from atomic_t to refcount_t
> >   net: convert inet_frag_queue.refcnt from atomic_t to refcount_t
> >   net: convert net.passive from atomic_t to refcount_t
> >   net: convert netlbl_lsm_cache.refcount from atomic_t to refcount_t
> >   net: convert packet_fanout.sk_ref from atomic_t to refcount_t
> 
> 
> Can you take a look at this please ?
> 
> Thanks.

Thank you very much for the report! This is an underflow (dec/sub from zero) 
that is reported by WARNING. 
I guess it is unlikely that actual code underflows, so the most probable cause 
is that it attempted to do refcount_inc/add() from zero, but then failed. 
However  in that case you should have seen another warning on refcount_inc() 
somewhere earlier. That one is actually the one I need to see to track the root 
cause. 
Could you tell me how do you arrive to the below output? Boot in what 
config/etc. 
I can try to reproduce to debug further. 

Best Regards,
Elena

> 
> [   64.601749] [ cut here ]
> [   64.601757] WARNING: CPU: 0 PID: 6476 at lib/refcount.c:184
> refcount_sub_and_test+0x75/0xa0
> [   64.601758] Modules linked in: w1_therm wire cdc_acm ehci_pci ehci_hcd
> mlx4_en ib_uverbs mlx4_ib ib_core mlx4_core
> [   64.601769] CPU: 0 PID: 6476 Comm: ip Tainted: GW   
> 4.12.0-smp-DEV #274
> [   64.601770] Hardware name: Intel RML,PCH/Iota_QC_19, BIOS 2.40.0 06/22/2016
> [   64.601771] task: 8837bf482040 task.stack: 8837bdc08000
> [   64.601773] RIP: 0010:refcount_sub_and_test+0x75/0xa0
> [   64.601774] RSP: 0018:8837bdc0f5c0 EFLAGS: 00010286
> [   64.601776] RAX: 0026 RBX: 0001 RCX:
> 
> [   64.601777] RDX: 0026 RSI: 0096 RDI:
> ed06f7b81eae
> [   64.601778] RBP: 8837bdc0f5d0 R08: 0004 R09: 
> fbfff4a54c25
> [   64.601779] R10: cbc500e5 R11: a52a6128 R12: 
> 881febcf6f24
> [   64.601779] R13: 881fbf4eaf00 R14: 881febcf6f80 R15: 
> 8837d7a4ed00
> [   64.601781] FS:  7ff5a2f6b700() GS:881fff80()
> knlGS:
> [   64.601782] CS:  0010 DS:  ES:  CR0: 80050033
> [   64.601783] CR2: 7ffcdc70d000 CR3: 001f9c91e000 CR4:
> 001406f0
> [   64.601783] Call Trace:
> [   64.601786]  refcount_dec_and_test+0x11/0x20
> [   64.601790]  fib_nl_delrule+0xc39/0x1630
> [   64.601793]  ? is_bpf_text_address+0xe/0x20
> [   64.601795]  ? fib_nl_newrule+0x25e0/0x25e0
> [   64.601798]  ? depot_save_stack+0x133/0x470
> [   64.601801]  ? ns_capable+0x13/0x20
> [   64.601803]  ? 

RE: [PATCH 00/17] v2 net generic subsystem refcount conversions

[Reshetova, Elena]

> From: Elena Reshetova 
> Date: Wed, 28 Jun 2017 14:54:49 +0300
> 
> > If there are no objections to the patches, please merge them via
> > respective trees.
> 
> This doesn't apply cleanly to the net-next tree, please respin.

Sorry, will rebase to the net-next and send new version today. The current 
version was on top of linux-next/stable. 

Best Regards,
Elena.

RE: [PATCH 16/16] drivers, net, intersil: convert request_context.refcount from atomic_t to refcount_t

[Reshetova, Elena]

> Elena Reshetova  writes:
> 
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  drivers/net/wireless/intersil/orinoco/orinoco_usb.c | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> The prefix should be "orinoco_usb:", I'll fix that.

Thanks for both! Will you take the patches in?

Best Regards,
Elena.

> 
> --
> Kalle Valo

RE: [PATCH 06/16] drivers, net, mlx5: convert mlx5_cq.refcount from atomic_t to refcount_t

[Reshetova, Elena]

> From: Elena Reshetova
> > Sent: 28 March 2017 09:57
> >
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> 
> I can't help feeling that you ought to find a scheme
> that will detect extra decrements and extra increments
> before the counter wraps 32 bits.
> 
> If an extra reference is requested every 100us it takes 4.8 days
> for the counter to increment back to zero.
> Simple tests aren't doing to find that - but it can easily happen
> on a system that is running for several years.

So, you are proposing to try detecting this case instead of preventing 
overflows?
Not sure how this would look like in a generic form...



> 
>   David

RE: [PATCH] net: convert sk_filter.refcnt from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/20/2017 10:37 AM, Elena Reshetova wrote:
> [...]
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index ebaeaf2..389cb8d 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -928,7 +928,7 @@ static void sk_filter_release_rcu(struct rcu_head *rcu)
> >*/
> >   static void sk_filter_release(struct sk_filter *fp)
> >   {
> > -   if (atomic_dec_and_test(>refcnt))
> > +   if (refcount_dec_and_test(>refcnt))
> > call_rcu(>rcu, sk_filter_release_rcu);
> >   }
> >
> > @@ -943,20 +943,27 @@ void sk_filter_uncharge(struct sock *sk, struct
> sk_filter *fp)
> >   /* try to charge the socket memory if there is space available
> >* return true on success
> >*/
> > -bool sk_filter_charge(struct sock *sk, struct sk_filter *fp)
> > +bool __sk_filter_charge(struct sock *sk, struct sk_filter *fp)
> 
> And this then becomes: static bool __sk_filter_charge(...)

Ups, I guess I am getting too tired of all these patch sendings, so more and 
more mistakes slide in. 
Will try switching to smth else to get attention back in order. 

> 
> >   {
> > u32 filter_size = bpf_prog_size(fp->prog->len);
> >
> > /* same check as in sock_kmalloc() */
> > if (filter_size <= sysctl_optmem_max &&
> > atomic_read(>sk_omem_alloc) + filter_size <
> sysctl_optmem_max) {
> > -   atomic_inc(>refcnt);
> > atomic_add(filter_size, >sk_omem_alloc);
> > return true;
> > }
> > return false;
> >   }
> 
> Since here is just all in slow-path, looks fine to me if the above
> is addressed as well in v3:
> 
> Acked-by: Daniel Borkmann 
> 
> Please make sure you add [PATCH net-next] in your subject in future
> so that it's clear which tree this goes to.

Thank you very much! Will do. 

Best Regards,
Elena.
> 
> Thanks,
> Daniel

RE: [PATCH 01/23] net, sunrpc: convert rpc_cred.cr_count from atomic_t to refcount_t

[Reshetova, Elena]

> On Fri, 2017-03-17 at 09:02 -0400, Jeff Layton wrote:
> > On Fri, 2017-03-17 at 12:50 +, Trond Myklebust wrote:
> > > On Fri, 2017-03-17 at 14:10 +0200, Elena Reshetova wrote:
> > > > refcount_t type and corresponding API should be
> > > > used instead of atomic_t when the variable is used as
> > > > a reference counter. This allows to avoid accidental
> > > > refcounter overflows that might lead to use-after-free
> > > > situations.
> > > >
> > > > Signed-off-by: Elena Reshetova 
> > > > Signed-off-by: Hans Liljestrand 
> > > > Signed-off-by: Kees Cook 
> > > > Signed-off-by: David Windsor 
> > > > ---
> > > >  include/linux/sunrpc/auth.h |  8 
> > > >  net/sunrpc/auth.c   | 12 ++--
> > > >  2 files changed, 10 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/include/linux/sunrpc/auth.h
> > > > b/include/linux/sunrpc/auth.h
> > > > index b1bc62b..bd36e0b 100644
> > > > --- a/include/linux/sunrpc/auth.h
> > > > +++ b/include/linux/sunrpc/auth.h
> > > > @@ -15,7 +15,7 @@
> > > >  #include 
> > > >  #include 
> > > >
> > > > -#include 
> > > > +#include 
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > @@ -68,7 +68,7 @@ struct rpc_cred {
> > > >  #endif
> > > >     unsigned long   cr_expire;  /* when
> > > > to gc
> > > > */
> > > >     unsigned long   cr_flags;   /* various
> > > > flags */
> > > > -   atomic_tcr_count;   /* ref count */
> > > > +   refcount_t  cr_count;   /* ref count
> > > > */
> > > >
> > >
> > > NACK. That's going to be hitting
> > > WARN_ONCE(!refcount_inc_not_zero(r),
> > > "refcount_t: increment on 0; use-after-free.\n") like there's no
> > > tomorrow...
> > >
> > > Please stop with these automated conversions. They are going to
> > > cause a
> > > lot more bugs than they fix.
> > >
> >
> > Agreed. These patchsets are touching places where we've already
> > banged
> > out most of the refcounting bugs. I'm against doing large scale
> > conversions like this without a damned good reason.
> >
> > I think it may be best to do this sort of thing in a more piecemeal
> > fashion. Pick a subsystem or two and do the conversions there to
> > prove
> > that they're better than what we have. If the subsystem already has
> > problems with its refcounting, then so much the better. Point to bugs
> > that this new infrastructure helped find.
> >
> > Encourage people to adopt your new infrastructure as new refcounted
> > objects are introduced into the kernel. You might even consider a LWN
> > article about this.
> >
> > Eventually we'll get around to changing existing code to use it, once
> > there is a sufficient advantage to doing so. Most likely when we're
> > reworking the code for other reasons, or when we're chasing some
> > horrid
> > refcounting bug and think that this might help find it.
> 
> The main issue is that this "refcount_t" implementation appears to be
> assuming that there is one and only one model for refcounts (the one
> where a value of "0" means "free me immediately").
> 
> The kernel has a plethora of object caching implementations where this
> is simply not the case; the dcache is a prime example, and this cache
> is another. In both these implementation, the atomic_t variable is
> being used more as a semaphore-style lock that prevents freeing of the
> object while it is in active use as opposed to being freeable, but
> cached. This is why these automated conversions are a nuisance and a
> source of bugs.

Ok, in this particular patch I agree that we missed that object is being reused 
(and yes there are many parts in kernel where similar thing happens as we 
learned from this exercise). 
Note that refcount_t implementation is fine with you "correctly" reusing your 
object:
i.e. when counter reaches zero, you take the object away from active use, but 
it might still stay in cache. 
BUT when you get a new object from cache you should initialize refcounter 
properly: set it to one vs. just do a "inc" on it. 
Problem really comes from this "increment me from zero". 

And the goal with these conversions is to take a look broadly on the kernel 
source and determine (with the feedback from maintainers who know code best, 
like your feedback now) what can be converted already now. 
Maintainers know their code and their usage of counters, so if it doesn't make 
sense to do it in a particular place (because of errors or other reasons), then 
it doesn't. 
But more we cover with new refcount_t, less chances we have with ever hitting 
refcounter bugs anywhere in the future. 

Best Regards,
Elena.

> 
> --
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> trond.mykleb...@primarydata.com

RE: [PATCH] net: convert sk_filter.refcnt from atomic_t to refcount_t

[Reshetova, Elena]

> Hello!
> 
> On 3/18/2017 3:58 PM, Elena Reshetova wrote:
> 
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> [...]
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index ebaeaf2..62267e2 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> [...]
> > @@ -1179,12 +1179,13 @@ static int __sk_attach_prog(struct bpf_prog *prog,
> struct sock *sk)
> > return -ENOMEM;
> >
> > fp->prog = prog;
> > -   atomic_set(>refcnt, 0);
> > +   refcount_set(>refcnt, 1);
> >
> > if (!sk_filter_charge(sk, fp)) {
> > kfree(fp);
> > return -ENOMEM;
> > }
> > +   refcount_set(>refcnt, 1);
> 
> Why do it twice?
> 
> [...]
> 
> MBR, Sergei

Sorry I just realized that I had unsquashed changes when I sent this patch, so 
this is still the old version. 
Will send a new one now. 

Best Regards,
Elena.

RE: [PATCH 08/17] net: convert sk_filter.refcnt from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/16/2017 04:28 PM, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >   include/linux/filter.h | 3 ++-
> >   net/core/filter.c  | 7 ---
> >   2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index 8053c38..20247e7 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -7,6 +7,7 @@
> >   #include 
> >
> >   #include 
> > +#include 
> >   #include 
> >   #include 
> >   #include 
> > @@ -431,7 +432,7 @@ struct bpf_prog {
> >   };
> >
> >   struct sk_filter {
> > -   atomic_trefcnt;
> > +   refcount_t  refcnt;
> > struct rcu_head rcu;
> > struct bpf_prog *prog;
> >   };
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index ebaeaf2..62267e2 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -928,7 +928,7 @@ static void sk_filter_release_rcu(struct rcu_head *rcu)
> >*/
> >   static void sk_filter_release(struct sk_filter *fp)
> >   {
> > -   if (atomic_dec_and_test(>refcnt))
> > +   if (refcount_dec_and_test(>refcnt))
> > call_rcu(>rcu, sk_filter_release_rcu);
> >   }
> >
> > @@ -950,7 +950,7 @@ bool sk_filter_charge(struct sock *sk, struct sk_filter 
> > *fp)
> > /* same check as in sock_kmalloc() */
> > if (filter_size <= sysctl_optmem_max &&
> > atomic_read(>sk_omem_alloc) + filter_size <
> sysctl_optmem_max) {
> > -   atomic_inc(>refcnt);
> > +   refcount_inc(>refcnt);
> > atomic_add(filter_size, >sk_omem_alloc);
> > return true;
> > }
> > @@ -1179,12 +1179,13 @@ static int __sk_attach_prog(struct bpf_prog *prog,
> struct sock *sk)
> > return -ENOMEM;
> >
> > fp->prog = prog;
> > -   atomic_set(>refcnt, 0);
> > +   refcount_set(>refcnt, 1);
> >
> > if (!sk_filter_charge(sk, fp)) {
> > kfree(fp);
> > return -ENOMEM;
> > }
> > +   refcount_set(>refcnt, 1);
> 
> Regarding the two subsequent refcount_set(, 1) that look a bit strange
> due to the sk_filter_charge() having refcount_inc() I presume ... can't
> the refcount API handle such corner case? 

Yes, it was exactly because of recount_inc() from zero in sk_filter_charge(). 
refcount_inc() would refuse to do an inc from zero for security reasons. At 
some 
point in past we discussed refcount_inc_not_one() but it was decided to be too 
special case
to support (we really have very little of such cases).


Or alternatively the let the
> sk_filter_charge() handle it, for example:
> 
> bool __sk_filter_charge(struct sock *sk, struct sk_filter *fp)
> {
>   u32 filter_size = bpf_prog_size(fp->prog->len);
> 
>   /* same check as in sock_kmalloc() */
>   if (filter_size <= sysctl_optmem_max &&
>   atomic_read(>sk_omem_alloc) + filter_size <
> sysctl_optmem_max) {
>   atomic_add(filter_size, >sk_omem_alloc);
>   return true;
>   }
>   return false;
> }
> 
> And this goes to filter.h:
> 
> bool __sk_filter_charge(struct sock *sk, struct sk_filter *fp);
> 
> bool sk_filter_charge(struct sock *sk, struct sk_filter *fp)
> {
>   bool ret = __sk_filter_charge(sk, fp);
>   if (ret)
>   refcount_inc(>refcnt);
>   return ret;
> }
> 
> ... and let __sk_attach_prog() call __sk_filter_charge() and only fo
> the second refcount_set()?
> 
> > old_fp = rcu_dereference_protected(sk->sk_filter,
> >
> lockdep_sock_is_held(sk));
> >

Oh, yes, this would make it look less awkward. Thank you for the suggestion 
Daniel! 
I guess we try to be less invasive for code changes overall, maybe even too 
careful... 

I will update the patch and send a new version. 

Best Regards,
Elena.

RE: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

[Reshetova, Elena]

> From: Kees Cook 
> Date: Thu, 16 Mar 2017 11:38:25 -0600
> 
> > I am, of course, biased, but I think the evidence of actual
> > refcounting attacks outweighs the theoretical performance cost of
> > these changes.
> 
> This is not theoretical at all.
> 
> We count the nanoseconds that every packet takes to get processed and
> you are adding quite a bit.
> 
> I understand your point of view, but this is knowingly going to add
> performance regressions to the networking code.

Should we then first measure the actual numbers to understand what we are 
talking here about? 
I would be glad to do it if you suggest what is the correct way to do 
measurements here to actually reflect the real life use cases. 

Best Regards,
Elena.

RE: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t

[Reshetova, Elena]

> On Tue, 2017-03-14 at 12:29 +0000, Reshetova, Elena wrote:
> > > Elena Reshetova <elena.reshet...@intel.com> writes:
> > >
> > > > refcount_t type and corresponding API should be
> > > > used instead of atomic_t when the variable is used as
> > > > a reference counter. This allows to avoid accidental
> > > > refcounter overflows that might lead to use-after-free
> > > > situations.
> > > >
> > > > Signed-off-by: Elena Reshetova <elena.reshet...@intel.com>
> > > > Signed-off-by: Hans Liljestrand <ishkam...@gmail.com>
> > > > Signed-off-by: Kees Cook <keesc...@chromium.org>
> > > > Signed-off-by: David Windsor <dwind...@gmail.com>
> > > > ---
> > > >  drivers/md/md.c | 6 +++---
> > > >  drivers/md/md.h | 3 ++-
> > > >  2 files changed, 5 insertions(+), 4 deletions(-)
> > >
> > > When booting linux-next (specifically 5be4921c9958ec) I'm seeing
> > > the
> > > backtrace below. I suspect this patch is just exposing an existing
> > > issue?
> >
> > Yes, we have actually been following this issue in the another
> > thread.
> > It looks like the object is re-used somehow, but I can't quite
> > understand how just by reading the code.
> > This was what I put into the previous thread:
> >
> > "The log below indicates that you are using your refcounter in a bit
> > weird way in mddev_find().
> > However, I can't find the place (just by reading the code) where you
> > would increment refcounter from zero (vs. setting it to one).
> > It looks like you either iterate over existing nodes (and increment
> > their counters, which should be >= 1 at the time of increment) or
> > create a new node, but then mddev_init() sets the counter to 1. "
> >
> > If you can help to understand what is going on with the object
> > creation/destruction, would be appreciated!
> >
> > Also Shaohua Li stopped this patch coming from his tree since the
> > issue was caught at that time, so we are not going to merge this
> > until we figure it out.
> 
> Asking on the correct list (dm-devel) would have got you the easy
> answer:  The refcount behind mddev->active is a genuine atomic.  It has
> refcount properties but only if the array fails to initialise (in that
> case, final put kills it).  Once it's added to the system as a gendisk,
> it cannot be freed until md_free().  Thus its ->active count can go to
> zero (when it becomes inactive; usually because of an unmount). On a
> simple allocation regardless of outcome, the last executed statement in
> md_alloc is mddev_put(): that destroys the device if we didn't manage
> to create it or returns 0 and adds an inactive device to the system
> which the user can get with mddev_find().

Thank you James for explaining this! I guess in this case, the conversion 
doesn't make sense. 
And sorry about not asking in a correct place: we are handling many similar 
patches now and while I try to reach the right audience using get_maintainer 
script, it doesn't always succeeds. 

Best Regards,
Elena.

> 
> James
> 

RE: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t

[Reshetova, Elena]

> Elena Reshetova  writes:
> 
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  drivers/md/md.c | 6 +++---
> >  drivers/md/md.h | 3 ++-
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> When booting linux-next (specifically 5be4921c9958ec) I'm seeing the
> backtrace below. I suspect this patch is just exposing an existing
> issue?

Yes, we have actually been following this issue in the another thread. 
It looks like the object is re-used somehow, but I can't quite understand how 
just by reading the code. 
This was what I put into the previous thread:

"The log below indicates that you are using your refcounter in a bit weird way 
in mddev_find(). 
However, I can't find the place (just by reading the code) where you would 
increment refcounter from zero (vs. setting it to one).
It looks like you either iterate over existing nodes (and increment their 
counters, which should be >= 1 at the time of increment) or create a new node, 
but then mddev_init() sets the counter to 1. "

If you can help to understand what is going on with the object 
creation/destruction, would be appreciated!

Also Shaohua Li stopped this patch coming from his tree since the issue was 
caught at that time, so we are not going to merge this until we figure it out. 

Best Regards,
Elena.

> 
> cheers
> 
> 
> [0.230738] md: Waiting for all devices to be available before autodetect
> [0.230742] md: If you don't use raid, use raid=noautodetect
> [0.230962] refcount_t: increment on 0; use-after-free.
> [0.230988] [ cut here ]
> [0.230996] WARNING: CPU: 0 PID: 1 at lib/refcount.c:114
> .refcount_inc+0x5c/0x70
> [0.231001] Modules linked in:
> [0.231006] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.11.0-rc1-gccN-next-
> 20170310-g5be4921 #1
> [0.231012] task: c0004940 task.stack: c0004944
> [0.231016] NIP: c05ac6bc LR: c05ac6b8 CTR:
> c0743390
> [0.231021] REGS: c00049443160 TRAP: 0700   Not tainted  (4.11.0-rc1-
> gccN-next-20170310-g5be4921)
> [0.231026] MSR: 80029032 
> [0.231033]   CR: 24024422  XER: 000c
> [0.231038] CFAR: c0a5356c SOFTE: 1
> [0.231038] GPR00: c05ac6b8 c000494433e0 c1079d00
> 002b
> [0.231038] GPR04:  00ef 
> c10418a0
> [0.231038] GPR08: 4af8 c0ecc9a8 c0ecc9a8
> 
> [0.231038] GPR12: 28024824 c6bb 
> c00049443a00
> [0.231038] GPR16:  c00049443a10 
> 
> [0.231038] GPR20:   c0f7dd20
> 
> [0.231038] GPR24: 014080c0 c12060b8 c1206080
> 0009
> [0.231038] GPR28: c0f7dde0 0090 
> c000461ae800
> [0.231100] NIP [c05ac6bc] .refcount_inc+0x5c/0x70
> [0.231104] LR [c05ac6b8] .refcount_inc+0x58/0x70
> [0.231108] Call Trace:
> [0.231112] [c000494433e0] [c05ac6b8] .refcount_inc+0x58/0x70
> (unreliable)
> [0.231120] [c00049443450] [c086c008]
> .mddev_find+0x1e8/0x430
> [0.231125] [c00049443530] [c0872b6c] .md_open+0x2c/0x140
> [0.231132] [c000494435c0] [c03962a4]
> .__blkdev_get+0xd4/0x520
> [0.231138] [c00049443690] [c0396cc0] .blkdev_get+0x1c0/0x4f0
> [0.231145] [c00049443790] [c0336d64]
> .do_dentry_open.isra.1+0x2a4/0x410
> [0.231152] [c00049443830] [c03523f4]
> .path_openat+0x624/0x1580
> [0.231157] [c00049443990] [c0354ce4]
> .do_filp_open+0x84/0x120
> [0.231163] [c00049443b10] [c0338d74]
> .do_sys_open+0x214/0x300
> [0.231170] [c00049443be0] [c0da69ac]
> .md_run_setup+0xa0/0xec
> [0.231176] [c00049443c60] [c0da4fbc]
> .prepare_namespace+0x60/0x240
> [0.231182] [c00049443ce0] [c0da47a8]
> .kernel_init_freeable+0x330/0x36c
> [0.231190] [c00049443db0] [c000dc44] .kernel_init+0x24/0x160
> [0.231197] [c00049443e30] [c000badc]
> .ret_from_kernel_thread+0x58/0x7c
> [0.231202] Instruction dump:
> [0.231206] 6000 3d22ffee 89296bfb 2f89 409effdc 3c62ffc6 3921
> 3d42ffee
> [0.231216] 38630928 992a6bfb 484a6e79 6000 <0fe0> 4bb8
> 6000 6000
> [

RE: [PATCH 22/29] drivers, scsi: convert iscsi_task.refcount from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/09/2017 08:18 AM, Reshetova, Elena wrote:
> >> On Mon, Mar 06, 2017 at 04:21:09PM +0200, Elena Reshetova wrote:
> >>> refcount_t type and corresponding API should be
> >>> used instead of atomic_t when the variable is used as
> >>> a reference counter. This allows to avoid accidental
> >>> refcounter overflows that might lead to use-after-free
> >>> situations.
> >>>
> >>> Signed-off-by: Elena Reshetova <elena.reshet...@intel.com>
> >>> Signed-off-by: Hans Liljestrand <ishkam...@gmail.com>
> >>> Signed-off-by: Kees Cook <keesc...@chromium.org>
> >>> Signed-off-by: David Windsor <dwind...@gmail.com>
> >>
> >> This looks OK to me.
> >>
> >> Acked-by: Chris Leech <cle...@redhat.com>
> >
> > Thank you for review! Do you have a tree that can take this change?
> 
> Hi Elena,
> 
> iscsi like fcoe should go via the SCSI tree.

Thanks Johannes! Should I resend with "Acked-by" added in order for it to be 
picked up? 

Best Regards,
Elena.


> 
> Byte,
>   Johannes
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

RE: [PATCH 22/29] drivers, scsi: convert iscsi_task.refcount from atomic_t to refcount_t

[Reshetova, Elena]

> On Mon, Mar 06, 2017 at 04:21:09PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> 
> This looks OK to me.
> 
> Acked-by: Chris Leech 

Thank you for review! Do you have a tree that can take this change? 

Best Regards,
Elena.

> 
> > ---
> >  drivers/scsi/libiscsi.c| 8 
> >  drivers/scsi/qedi/qedi_iscsi.c | 2 +-
> >  include/scsi/libiscsi.h| 3 ++-
> >  3 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> > index 834d121..7eb1d2c 100644
> > --- a/drivers/scsi/libiscsi.c
> > +++ b/drivers/scsi/libiscsi.c
> > @@ -516,13 +516,13 @@ static void iscsi_free_task(struct iscsi_task *task)
> >
> >  void __iscsi_get_task(struct iscsi_task *task)
> >  {
> > -   atomic_inc(>refcount);
> > +   refcount_inc(>refcount);
> >  }
> >  EXPORT_SYMBOL_GPL(__iscsi_get_task);
> >
> >  void __iscsi_put_task(struct iscsi_task *task)
> >  {
> > -   if (atomic_dec_and_test(>refcount))
> > +   if (refcount_dec_and_test(>refcount))
> > iscsi_free_task(task);
> >  }
> >  EXPORT_SYMBOL_GPL(__iscsi_put_task);
> > @@ -744,7 +744,7 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct
> iscsi_hdr *hdr,
> >  * released by the lld when it has transmitted the task for
> >  * pdus we do not expect a response for.
> >  */
> > -   atomic_set(>refcount, 1);
> > +   refcount_set(>refcount, 1);
> > task->conn = conn;
> > task->sc = NULL;
> > INIT_LIST_HEAD(>running);
> > @@ -1616,7 +1616,7 @@ static inline struct iscsi_task 
> > *iscsi_alloc_task(struct
> iscsi_conn *conn,
> > sc->SCp.phase = conn->session->age;
> > sc->SCp.ptr = (char *) task;
> >
> > -   atomic_set(>refcount, 1);
> > +   refcount_set(>refcount, 1);
> > task->state = ISCSI_TASK_PENDING;
> > task->conn = conn;
> > task->sc = sc;
> > diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
> > index b9f79d3..3895bd5 100644
> > --- a/drivers/scsi/qedi/qedi_iscsi.c
> > +++ b/drivers/scsi/qedi/qedi_iscsi.c
> > @@ -1372,7 +1372,7 @@ static void qedi_cleanup_task(struct iscsi_task *task)
> >  {
> > if (!task->sc || task->state == ISCSI_TASK_PENDING) {
> > QEDI_INFO(NULL, QEDI_LOG_IO, "Returning
> ref_cnt=%d\n",
> > - atomic_read(>refcount));
> > + refcount_read(>refcount));
> > return;
> > }
> >
> > diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> > index b0e275d..24d74b5 100644
> > --- a/include/scsi/libiscsi.h
> > +++ b/include/scsi/libiscsi.h
> > @@ -29,6 +29,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -139,7 +140,7 @@ struct iscsi_task {
> >
> > /* state set/tested under session->lock */
> > int state;
> > -   atomic_trefcount;
> > +   refcount_t  refcount;
> > struct list_headrunning;/* running cmd list */
> > void*dd_data;   /*
> driver/transport data */
> >  };
> > --
> > 2.7.4
> >

RE: [Xen-devel] [PATCH 29/29] drivers, xen: convert grant_map.users from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/08/2017 08:49 AM, Reshetova, Elena wrote:
> >> On 03/06/2017 09:21 AM, Elena Reshetova wrote:
> >>> refcount_t type and corresponding API should be
> >>> used instead of atomic_t when the variable is used as
> >>> a reference counter. This allows to avoid accidental
> >>> refcounter overflows that might lead to use-after-free
> >>> situations.
> >>>
> >>> Signed-off-by: Elena Reshetova <elena.reshet...@intel.com>
> >>> Signed-off-by: Hans Liljestrand <ishkam...@gmail.com>
> >>> Signed-off-by: Kees Cook <keesc...@chromium.org>
> >>> Signed-off-by: David Windsor <dwind...@gmail.com>
> >>> ---
> >>>  drivers/xen/gntdev.c | 11 ++-
> >>>  1 file changed, 6 insertions(+), 5 deletions(-)
> >> Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>
> > Is there a tree that can take this change? Turns out it is better to 
> > propagate
> changes via separate trees and only leftovers can be taken via Greg's tree.
> >
> 
> Sure, we can take it via Xen tree for rc3.

Thank you very much!

Best Regards,
Elena.

> 
> -boris

RE: [Xen-devel] [PATCH 29/29] drivers, xen: convert grant_map.users from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/06/2017 09:21 AM, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  drivers/xen/gntdev.c | 11 ++-
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Boris Ostrovsky 

Is there a tree that can take this change? Turns out it is better to propagate 
changes via separate trees and only leftovers can be taken via Greg's tree.  

Best Regards,
Elena.

RE: [PATCH 21/29] drivers, s390: convert fc_fcp_pkt.ref_cnt from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/06/2017 03:21 PM, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> 
> The subject is wrong, should be something like "scsi: libfc convert
> fc_fcp_pkt.ref_cnt from atomic_t to refcount_t" but not s390.
> 
> Other than that
> Acked-by: Johannes Thumshirn 

Turns out that it is better that all these patches go through the respective 
maintainer trees, if present. 
If I send an updated patch (with subject fixed), could you merge it through 
your tree?

Best Regards,
Elena.
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

RE: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t

[Reshetova, Elena]

> On Mon, Mar 06, 2017 at 04:20:55PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> 
> Looks good. Let me know how do you want to route the patch to upstream.

Greg, you previously mentioned that driver's conversions can go via your tree. 
Does this still apply?
Or should I be asking maintainers to merge these patches via their trees? 
I am not sure about the correct (and easier for everyone) way, please suggest.  

Best Regards,
Elena.
> 
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  drivers/md/md.c | 6 +++---
> >  drivers/md/md.h | 3 ++-
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 985374f..94c8ebf 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -449,7 +449,7 @@ EXPORT_SYMBOL(md_unplug);
> >
> >  static inline struct mddev *mddev_get(struct mddev *mddev)
> >  {
> > -   atomic_inc(>active);
> > +   refcount_inc(>active);
> > return mddev;
> >  }
> >
> > @@ -459,7 +459,7 @@ static void mddev_put(struct mddev *mddev)
> >  {
> > struct bio_set *bs = NULL;
> >
> > -   if (!atomic_dec_and_lock(>active, _mddevs_lock))
> > +   if (!refcount_dec_and_lock(>active, _mddevs_lock))
> > return;
> > if (!mddev->raid_disks && list_empty(>disks) &&
> > mddev->ctime == 0 && !mddev->hold_active) {
> > @@ -495,7 +495,7 @@ void mddev_init(struct mddev *mddev)
> > INIT_LIST_HEAD(>all_mddevs);
> > setup_timer(>safemode_timer, md_safemode_timeout,
> > (unsigned long) mddev);
> > -   atomic_set(>active, 1);
> > +   refcount_set(>active, 1);
> > atomic_set(>openers, 0);
> > atomic_set(>active_io, 0);
> > spin_lock_init(>lock);
> > diff --git a/drivers/md/md.h b/drivers/md/md.h
> > index b8859cb..4811663 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> > @@ -22,6 +22,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -360,7 +361,7 @@ struct mddev {
> >  */
> > struct mutexopen_mutex;
> > struct mutexreconfig_mutex;
> > -   atomic_tactive;
>   /* general refcount */
> > +   refcount_t  active;
>   /* general refcount */
> > atomic_topeners;/*
> number of active opens */
> >
> > int
>   changed;/* True if we might need to
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 10/29] drivers, md: convert stripe_head.count from atomic_t to refcount_t

[Reshetova, Elena]

> On Mon, Mar 06, 2017 at 04:20:57PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  drivers/md/raid5-cache.c |  8 +++---
> >  drivers/md/raid5.c   | 66 
> > 
> >  drivers/md/raid5.h   |  3 ++-
> >  3 files changed, 39 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> > index 3f307be..6c05e12 100644
> > --- a/drivers/md/raid5-cache.c
> > +++ b/drivers/md/raid5-cache.c
> 
> snip
> >sh->check_state, sh->reconstruct_state);
> >
> > analyse_stripe(sh, );
> > @@ -4924,7 +4924,7 @@ static void activate_bit_delay(struct r5conf *conf,
> > struct stripe_head *sh = list_entry(head.next, struct
> stripe_head, lru);
> > int hash;
> > list_del_init(>lru);
> > -   atomic_inc(>count);
> > +   refcount_inc(>count);
> > hash = sh->hash_lock_index;
> > __release_stripe(conf, sh,
> _inactive_list[hash]);
> > }
> > @@ -5240,7 +5240,7 @@ static struct stripe_head 
> > *__get_priority_stripe(struct
> r5conf *conf, int group)
> > sh->group = NULL;
> > }
> > list_del_init(>lru);
> > -   BUG_ON(atomic_inc_return(>count) != 1);
> > +   BUG_ON(refcount_inc_not_zero(>count));
> 
> This changes the behavior. refcount_inc_not_zero doesn't inc if original 
> value is 0

Hm.. So, you want to inc here in any case and BUG if the end result differs 
from 1. 
So essentially you want to only increment here from zero to one under normal 
conditions... This is a challenge for refcount_t and against the design.
Is it ok just to maybe do this here:

-   BUG_ON(atomic_inc_return(>count) != 1);
+   BUG_ON(refcount_read(>count) != 0);
+   refcount_set((>count, 1);

Do we have an issue with locking in this case? Or maybe it is then better to 
leave this one to be atomic_t without protection since it isn't a real 
refcounter as it turns out. 

Best Regards,
Elena. 

> 
> Thanks,
> Shaohua

RE: [PATCH 13/29] drivers, media: convert vb2_vmarea_handler.refcount from atomic_t to refcount_t

[Reshetova, Elena]

> Hi Elena,
> 
> On Mon, Mar 06, 2017 at 04:21:00PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> >  drivers/media/v4l2-core/videobuf2-memops.c | 6 +++---
> >  include/media/videobuf2-memops.h   | 3 ++-
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/videobuf2-memops.c 
> > b/drivers/media/v4l2-
> core/videobuf2-memops.c
> > index 1cd322e..4bb8424 100644
> > --- a/drivers/media/v4l2-core/videobuf2-memops.c
> > +++ b/drivers/media/v4l2-core/videobuf2-memops.c
> > @@ -96,10 +96,10 @@ static void vb2_common_vm_open(struct
> vm_area_struct *vma)
> > struct vb2_vmarea_handler *h = vma->vm_private_data;
> >
> > pr_debug("%s: %p, refcount: %d, vma: %08lx-%08lx\n",
> > -  __func__, h, atomic_read(h->refcount), vma->vm_start,
> > +  __func__, h, refcount_read(h->refcount), vma->vm_start,
> >vma->vm_end);
> >
> > -   atomic_inc(h->refcount);
> > +   refcount_inc(h->refcount);
> >  }
> >
> >  /**
> > @@ -114,7 +114,7 @@ static void vb2_common_vm_close(struct
> vm_area_struct *vma)
> > struct vb2_vmarea_handler *h = vma->vm_private_data;
> >
> > pr_debug("%s: %p, refcount: %d, vma: %08lx-%08lx\n",
> > -  __func__, h, atomic_read(h->refcount), vma->vm_start,
> > +  __func__, h, refcount_read(h->refcount), vma->vm_start,
> >vma->vm_end);
> >
> > h->put(h->arg);
> > diff --git a/include/media/videobuf2-memops.h b/include/media/videobuf2-
> memops.h
> > index 36565c7a..a6ed091 100644
> > --- a/include/media/videobuf2-memops.h
> > +++ b/include/media/videobuf2-memops.h
> > @@ -16,6 +16,7 @@
> >
> >  #include 
> >  #include 
> > +#include 
> >
> >  /**
> >   * struct vb2_vmarea_handler - common vma refcount tracking handler
> > @@ -25,7 +26,7 @@
> >   * @arg:   argument for @put callback
> >   */
> >  struct vb2_vmarea_handler {
> > -   atomic_t*refcount;
> > +   refcount_t  *refcount;
> 
> This is a pointer to refcount, not refcount itself. The refcount is part of
> a memory type specific struct, the types that you change in the following
> three patches. I guess it would still compile and work as separate patches
> but you'd sure get warnings at least.

Actually it doesn't compile without this patch if I remember it correctly back 
in past when I was initially splitting the patches per variable. 

> 
> How about merging this and the three following patches that change the memop
> refcount types?

Sounds good!

Best Regards,
Elena.
> 
> > void(*put)(void *arg);
> > void*arg;
> >  };
> 
> --
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi   XMPP: sai...@retiisi.org.uk

RE: [PATCH 12/29] drivers, media: convert s2255_dev.num_channels from atomic_t to refcount_t

[Reshetova, Elena]

> Hi Elena,
> 
> On Mon, Mar 06, 2017 at 04:20:59PM +0200, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> > ---
> ...
> > @@ -1688,7 +1689,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev)
> > "failed to register
> video device!\n");
> > break;
> > }
> > -   atomic_inc(>num_channels);
> > +   refcount_set(>num_channels, 1);
> 
> That's not right. The loop runs four iterations and the value after the
> loop should be indeed the number of channels.

Oh, yes, I was blind here, sorry. The problem why it cannot be left as inc is 
because it would do increment from zero here, which is not allowed by 
refcount_t interface. 

> atomic_t isn't really used for reference counting here, but storing out how
> many "channels" there are per hardware device, with maximum number of four
> (4). I'd leave this driver using atomic_t.
Yes, sounds like the best thing to do. I will drop this patch. 

Thank you for reviews!

Best Regards,
Elena.
> 
> > v4l2_info(>v4l2_dev, "V4L2 device registered
> as %s\n",
> >   video_device_node_name(
> >vdev));
> >
> 
> --
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi   XMPP: sai...@retiisi.org.uk

RE: [PATCH 21/29] drivers, s390: convert fc_fcp_pkt.ref_cnt from atomic_t to refcount_t

[Reshetova, Elena]

> On 03/06/2017 03:21 PM, Elena Reshetova wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> 
> The subject is wrong, should be something like "scsi: libfc convert
> fc_fcp_pkt.ref_cnt from atomic_t to refcount_t" but not s390.

Very sorry about this: the error in the subject got from the time when I was 
trying to break the bigger drivers patch set into per-variable part and trying 
to automate the process too much :(
I will fix it in the end version!

Best Regards,
Elena

> 
> Other than that
> Acked-by: Johannes Thumshirn 
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

RE: [PATCH 11/29] drivers, media: convert cx88_core.refcount from atomic_t to refcount_t

[Reshetova, Elena]

> Hello.
> 
> On 03/06/2017 05:20 PM, Elena Reshetova wrote:
> 
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Elena Reshetova 
> > Signed-off-by: Hans Liljestrand 
> > Signed-off-by: Kees Cook 
> > Signed-off-by: David Windsor 
> [...]
> > diff --git a/drivers/media/pci/cx88/cx88.h b/drivers/media/pci/cx88/cx88.h
> > index 115414c..16c1313 100644
> > --- a/drivers/media/pci/cx88/cx88.h
> > +++ b/drivers/media/pci/cx88/cx88.h
> > @@ -24,6 +24,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include 
> >  #include 
> > @@ -339,7 +340,7 @@ struct cx8802_dev;
> >
> >  struct cx88_core {
> > struct list_head   devlist;
> > -   atomic_t   refcount;
> > +   refcount_t   refcount;
> 
> Could you please keep the name aligned with above and below?

You mean "not aligned" to devlist, but with a shift like it was before? 
Sure, will fix. Is the patch ok otherwise?

Best Regards,
Elena.

> 
> >
> > /* board name */
> > intnr;
> >
> 
> MBR, Sergei

RE: [RFC 3/4] security/checmate: Add Checmate sample

[Reshetova, Elena]

Sorry have to resend from normal mail client due to gmail stupid interface. I 
am not able to find plain text button anywhere anymore...

>On Fri, 5 Aug 2016 at 13:49 Elena Reshetova  
>wrote:

>The Checmate sample installs a policy barring new AF_INET connections
>to port 1. We install the hook, and show an example of connect
>returning EPERM, and then reset the policy.

Could you actually provide a more complex example, where you actually need to 
share a state between different hooks?
Even minor LSMs need to keep internal info in one way or another to make 
useful decisions. Like Yama needs to track ptrace info, Hardchroot would need 
to track if process is inside chroot or not and etc.


smime.p7s
Description: S/MIME cryptographic signature