On 9/24/21 7:21 PM, Tejun Heo wrote:
On Thu, Sep 23, 2021 at 10:09:23PM +0200, Daniel Borkmann wrote:If cgroup_sk_alloc() is called from interrupt context, then just assign the root cgroup to skcd->cgroup. Prior to commit 8520e224f547 ("bpf, cgroups: Fix cgroup v2 fallback on v1/v2 mixed mode") we would just return, and later on in sock_cgroup_ptr(), we were NULL-testing the cgroup in fast-path. Rather than re-adding the NULL-test to the fast-path we can just assign it once from cgroup_sk_alloc() given v1/v2 handling has been simplified.I think you should explain why this is safe - ie. when do we hit the condition and leak the socket to the root cgroup and why is that okay?
I'll add it to the commit log. What I was trying to say is that before the 8520e224f547 fix, the cgroup_sk_alloc() would bail out and return early when in_interrupt(), and in that case skcd->cgroup remained NULL. sock_cgroup_ptr() had a NULL check for it in the old code where it says 'v ?: &cgrp_dfl_root.cgrp' and I wonder given the !CONFIG_CGROUP_NET_PRIO && !CONFIG_CGROUP_NET_CLASSID path doesn't have it, whether syzbot never tripped over it because it was not testing with such config. From the repro [0], this is specific to old netrom legacy code where RX handler nr_rx_frame() calls nr_make_new() which calls sk_alloc() and therefore cgroup_sk_alloc() with in_interrupt() condition. Thus the NULL skcd->cgroup, where it trips over on cgroup_sk_free() side [1]. I'm certain you hit the same in netrom with mentioned configs off. Thanks, Daniel [0] https://syzkaller.appspot.com/x/repro.syz?x=12f2b28d300000 [1] https://lkml.org/lkml/2021/9/17/1041
