On 9/9/21 6:47 PM, sdf@xxxxxxxxxx wrote:
On 09/09, Daniel Borkmann wrote:
[...]
static inline struct cgroup *sock_cgroup_ptr(struct sock_cgroup_data *skcd) { -#if defined(CONFIG_CGROUP_NET_PRIO) || defined(CONFIG_CGROUP_NET_CLASSID) - unsigned long v; - - /* - * @skcd->val is 64bit but the following is safe on 32bit too as we - * just need the lower ulong to be written and read atomically. - */ - v = READ_ONCE(skcd->val); - - if (v & 3) - return &cgrp_dfl_root.cgrp; - - return (struct cgroup *)(unsigned long)v ?: &cgrp_dfl_root.cgrp; -#else - return (struct cgroup *)(unsigned long)skcd->val; -#endif + return READ_ONCE(skcd->cgroup);Do we really need READ_ONCE here? I was always assuming it was there because we were flipping that lower bit. Now that it's a simple pointer, why not 'return skcd->cgroup' instead?
Hm, good point, from cgroup_sk_alloc() side we don't need it as struct sock is not public yet at that point, I'll send a v2 and remove the READ_ONCE()/WRITE_ONCE() pair for the cgroup pointer. Thanks for spotting! Daniel
