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

Groups > linux.kernel > #1624783

Re: [PATCH linux 2/2] net sched actions: fix refcount decrement on error

Show all headers | View raw

On Sat, Apr 15, 2017 at 11:48 AM, Wolfgang Bumiller
<w.bumiller@proxmox.com> wrote:
>
>> On April 15, 2017 at 8:20 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>
>>
>> On Fri, Apr 14, 2017 at 2:08 AM, Wolfgang Bumiller
>> <w.bumiller@proxmox.com> wrote:
>> > Before I do that - trying to wrap my head around the interdependencies
>> > here better to be thorough - I noticed that tcf_hash_release() can
>> > return ACT_P_DELETED. The ACT_P_CREATED case means tcf_hash_create()
>> > was used, in the other case the tc_action's ref & bind count is bumped
>> > by tcf_hash_check() and then also decremented by tcf_hash_release() if
>> > it existed, iow. kept at 1, but not always: It does always happen in
>> > act_police.c but in other files such as act_bpf.c or act_connmark.c if
>> > eg. bind is set they return without decrementing, so both ref&bind count
>> > are bumped when they return - the refcount logic isn't easy to follow
>> > for a newcomer. Now there are two uses of __tcf_hash_release() in
>> > act_api.c which check for a return value of ACT_P_DELETED, in which case
>> > they call module_put().
>>
>>
>> That's the nasty part... IIRC, Jamal has fixed two bugs on action refcnt'ing.
>> We really need to clean up the code.
>>
>> > So I'm not sure exactly how the module and tc_action counts are related
>> > (and I usually like to understand my own patches ;-) ).
>>
>>
>> Each action holds a refcnt to its module, each filter holds a refcnt to
>> its bound or referenced (unbound) action.
>>
>>
>> > Maybe I'm missing something obvious but I'm currently a bit confused as
>> > to whether the tcf_hash_release() call there is okay, or should have its
>> > return value checked or should depend on ->init()'s ACT_P_CREATED value
>> > as well?
>> >
>>
>> I think it's the same? If we have ACT_P_CREATED here, tcf_hash_release()
>> will return ACT_P_DELETED for sure because the newly created action has
>> refcnt==1?
>
> Makes sense on the one hand, but for ACT_P_DELETED both ref and bind
> count need to reach 0, so I'm still concerned that the different behaviors

Bind refcnt is only used when it is bound to a filter and refcnt is always used,
so either bind refcnt is 0 or it is same with refcnt.

> I mentioned above might be problematic if we use ACT_P_CREATED only.
> (It also means my patches still leak a count - which is probably still
> better than the previous underflow, but ultimately doesn't satisfy me.)
> Should I still resend it this way for the record with the Acked-bys?
> (Since given the fact that with unprivileged containers it's possible to
> trigger this access and potentially crash the kernel I strongly feel that
> some version of this should end up in the 4.11 release.)

I think so.

Thanks.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[RFC PATCH linux 0/2] net sched actions: access to uninitialized data and error handling Wolfgang Bumiller <w.bumiller@proxmox.com> - 2017-04-12 16:40 +0200
  [PATCH linux 2/2] net sched actions: fix refcount decrement on error Wolfgang Bumiller <w.bumiller@proxmox.com> - 2017-04-12 16:40 +0200
    Re: [PATCH linux 2/2] net sched actions: fix refcount decrement on error Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-13 06:30 +0200
      Re: [PATCH linux 2/2] net sched actions: fix refcount decrement on  error Wolfgang Bumiller <w.bumiller@proxmox.com> - 2017-04-13 10:10 +0200
        Re: [PATCH linux 2/2] net sched actions: fix refcount decrement on error Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-13 20:10 +0200
          Re: [PATCH linux 2/2] net sched actions: fix refcount decrement on  error Wolfgang Bumiller <w.bumiller@proxmox.com> - 2017-04-14 11:10 +0200
            Re: [PATCH linux 2/2] net sched actions: fix refcount decrement on error Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-15 20:30 +0200
              Re: [PATCH linux 2/2] net sched actions: fix refcount decrement on  error Wolfgang Bumiller <w.bumiller@proxmox.com> - 2017-04-15 20:50 +0200
                Re: [PATCH linux 2/2] net sched actions: fix refcount decrement on error Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-17 20:20 +0200
    Re: [PATCH linux 2/2] net sched actions: fix refcount decrement on error Roman Mashak <mrv@mojatatu.com> - 2017-04-13 15:50 +0200
  Re: [RFC PATCH linux 0/2] net sched actions: access to uninitialized  data and error handling Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-13 03:30 +0200
  Re: [RFC PATCH linux 0/2] net sched actions: access to  uninitialized data and error handling David Miller <davem@davemloft.net> - 2017-04-17 17:10 +0200

csiph-web