On 9/24/21 7:21 PM, Tejun Heo wrote:

On Thu, Sep 23, 2021 at 10:09:23PM +0200, Daniel Borkmann wrote:

quoted

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