netdev - Re: [PATCH net-next] net/sched: cls_flower: verify root pointer before dereferncing it

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20161124.152546.1174938340314080043.davem@davemloft.net>
Date:   Thu, 24 Nov 2016 15:25:46 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     xiyou.wangcong@...il.com
Cc:     jiri@...nulli.us, daniel@...earbox.net, roid@...lanox.com,
        netdev@...r.kernel.org, jiri@...lanox.com, ogerlitz@...lanox.com,
        cwang@...pensource.com, john.fastabend@...il.com
Subject: Re: [PATCH net-next] net/sched: cls_flower: verify root pointer
 before dereferncing it

From: Cong Wang <xiyou.wangcong@...il.com>
Date: Tue, 22 Nov 2016 11:28:37 -0800

> On Tue, Nov 22, 2016 at 8:11 AM, Jiri Pirko <jiri@...nulli.us> wrote:
>> Tue, Nov 22, 2016 at 05:04:11PM CET, daniel@...earbox.net wrote:
>>>Hmm, I don't think we want to have such an additional test in fast
>>>path for each and every classifier. Can we think of ways to avoid that?
>>>
>>>My question is, since we unlink individual instances from such tp-internal
>>>lists through RCU and release the instance through call_rcu() as well as
>>>the head (tp->root) via kfree_rcu() eventually, against what are we protecting
>>>setting RCU_INIT_POINTER(tp->root, NULL) in ->destroy() callback? Something
>>>not respecting grace period?
>>
>> If you call tp->ops->destroy in call_rcu, you don't have to set tp->root
>> to null.
> 
> We do need to respect the grace period if we touch the globally visible
> data structure tp in tcf_destroy(). Therefore Roi's patch is not fixing the
> right place.

Another idea is to assign tp->root to a dummy static cls_fl_head object,
instead of NULL, which we just make sure has an ht.elems value of zero.

This avoids having to touch the fast path and also avoids all of these
complicated changes being discussed wrt. doing things in call_rcu_bh()
or whatever.