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

Groups > linux.kernel > #1523799 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  Re: [RFC][PATCH 2/7] kref: Add kref_read() Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-11-16 21:10 +0100
    Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 10:00 +0100
      Re: [RFC][PATCH 2/7] kref: Add kref_read() Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-11-17 18:30 +0100
        Re: [RFC][PATCH 2/7] kref: Add kref_read() Thomas Gleixner <tglx@linutronix.de> - 2016-11-17 18:30 +0100
        RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-18 18:40 +0100
          Re: [RFC][PATCH 2/7] kref: Add kref_read() Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-11-19 04:50 +0100
            RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-21 09:20 +0100
          Re: [RFC][PATCH 2/7] kref: Add kref_read() David Windsor <dwindsor@gmail.com> - 2016-11-21 13:50 +0100
            RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-21 16:40 +0100
              Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-21 16:50 +0100
                Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-21 17:10 +0100
                  RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-21 20:30 +0100
                    Re: [RFC][PATCH 2/7] kref: Add kref_read() David Windsor <dwindsor@gmail.com> - 2016-11-21 21:20 +0100
                      Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-22 11:40 +0100

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

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

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.

[toc] | [next] | [standalone]

#1524168

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/20161115104608.GH3142@twins.programming.kicks-ass.net

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.

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

#1524546

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/20161115104608.GH3142@twins.programming.kicks-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.

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

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

#1524547

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/20161115104608.GH3142@twins.programming.kicks-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.
> 
> > 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

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

#1525581

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/20161115104608.GH3142@twins.programming.kicks
> -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... 

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

#1525841

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/20161115104608.GH3142@twins.programming.kicks
> > -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.

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

#1526438

> 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/20161115104608.GH3142@twins.programming.kicks
> > > -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?

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

#1526642

On Fri, Nov 18, 2016 at 12:33 PM, Reshetova, Elena
<elena.reshetova@intel.com> 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/20161115104608.GH3142@twins.programming.kicks
>> -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().

>
>

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

#1526796

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

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

#1526815

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.

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

#1526855

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.

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

#1527000

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

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

#1527019

On Mon, Nov 21, 2016 at 2:27 PM, Reshetova, Elena
<elena.reshetova@intel.com> 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;

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

#1527391

On Mon, Nov 21, 2016 at 03:12:33PM -0500, David Windsor wrote:
> On Mon, Nov 21, 2016 at 2:27 PM, Reshetova, Elena
> <elena.reshetova@intel.com> 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.

Now, arguably the 1->0 case is special, and we can provide limited
support for that, but I'd be hesitant to provide the full cmpxchg.

We could for instance provide: refcount_dec_if_one().

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

So it really depends on what the network guys are willing to put up
with, if their primary goal is to avoid the SHARED state, we could add a
load-exclusive. But I suspect they'd not be happy with that either...

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

That cannot actually work in the presence of actual atomic instructions
not serialized by that lock.

Also, the network guys will absolutely kill you if you propose something
like that.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web