Re: [PATCH net-next] net: sock: add the case if sk is NULL

On 8/9/21 8:12 AM, yajun.deng@linux.dev wrote:

August 6, 2021 9:11 PM, "Jakub Kicinski" [off-list ref] wrote:

quoted

On Fri, 6 Aug 2021 14:38:15 +0800 Yajun Deng wrote:

quoted

Add the case if sk is NULL in sock_{put, hold},
The caller is free to use it.

Signed-off-by: Yajun Deng <redacted>

The obvious complaint about this patch (and your previous netdev patch)
is that you're spraying branches everywhere in the code. Sure, it may

Sorry for that, I'll be more normative in later submission.

quoted

be okay for free(), given how expensive of an operation that is but
is having refcounting functions accept NULL really the best practice?

Can you give us examples in the kernel where that's the case?

0   include/net/neighbour.h         neigh_clone()
1   include/linux/cgroup.h          get_cgroup_ns() and put_cgroup_ns()  (This is very similar to my submission)
2   include/linux/ipc_namespace.h   get_ipc_ns()
3   include/linux/posix_acl.h       posix_acl_dup()
4   include/linux/pid.h             get_pid()
5   include/linux/user_namespace.h  get_user_ns()

These helpers might be called with NULL pointers by design.

sock_put() and sock_hold() are never called with NULL.

Same for put_page() and hundreds of other functions.

By factorizing a conditional in the function, hoping to remove one in few callers,
we add more conditional branches (and increase code size)