* [PATCH net-next] net: sock: add the case if sk is NULL
@ 2021-08-06 6:38 Yajun Deng
2021-08-06 13:11 ` Jakub Kicinski
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Yajun Deng @ 2021-08-06 6:38 UTC (permalink / raw)
To: davem, kuba; +Cc: netdev, linux-kernel, Yajun Deng
Add the case if sk is NULL in sock_{put, hold},
The caller is free to use it.
Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
include/net/sock.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 6e761451c927..8821ec0d4147 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -699,7 +699,8 @@ static inline bool __sk_del_node_init(struct sock *sk)
static __always_inline void sock_hold(struct sock *sk)
{
- refcount_inc(&sk->sk_refcnt);
+ if (sk)
+ refcount_inc(&sk->sk_refcnt);
}
/* Ungrab socket in the context, which assumes that socket refcnt
@@ -1811,7 +1812,7 @@ void sock_init_data(struct socket *sock, struct sock *sk);
/* Ungrab socket and destroy it, if it was the last reference. */
static inline void sock_put(struct sock *sk)
{
- if (refcount_dec_and_test(&sk->sk_refcnt))
+ if (sk && refcount_dec_and_test(&sk->sk_refcnt))
sk_free(sk);
}
/* Generic version of sock_put(), dealing with all sockets
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: sock: add the case if sk is NULL
2021-08-06 6:38 [PATCH net-next] net: sock: add the case if sk is NULL Yajun Deng
@ 2021-08-06 13:11 ` Jakub Kicinski
2021-08-09 6:12 ` yajun.deng
2021-08-09 9:15 ` Eric Dumazet
2 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2021-08-06 13:11 UTC (permalink / raw)
To: Yajun Deng; +Cc: davem, netdev, linux-kernel
On Fri, 6 Aug 2021 14:38:15 +0800 Yajun Deng wrote:
> Add the case if sk is NULL in sock_{put, hold},
> The caller is free to use it.
>
> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
The obvious complaint about this patch (and your previous netdev patch)
is that you're spraying branches everywhere in the code. Sure, it may
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: sock: add the case if sk is NULL
2021-08-06 6:38 [PATCH net-next] net: sock: add the case if sk is NULL Yajun Deng
2021-08-06 13:11 ` Jakub Kicinski
@ 2021-08-09 6:12 ` yajun.deng
2021-08-09 9:34 ` Eric Dumazet
2021-08-09 9:15 ` Eric Dumazet
2 siblings, 1 reply; 6+ messages in thread
From: yajun.deng @ 2021-08-09 6:12 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, linux-kernel
August 6, 2021 9:11 PM, "Jakub Kicinski" <kuba@kernel.org> wrote:
> On Fri, 6 Aug 2021 14:38:15 +0800 Yajun Deng wrote:
>
>> Add the case if sk is NULL in sock_{put, hold},
>> The caller is free to use it.
>>
>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
>
> 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.
> 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()
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: sock: add the case if sk is NULL
2021-08-06 6:38 [PATCH net-next] net: sock: add the case if sk is NULL Yajun Deng
2021-08-06 13:11 ` Jakub Kicinski
2021-08-09 6:12 ` yajun.deng
@ 2021-08-09 9:15 ` Eric Dumazet
2 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2021-08-09 9:15 UTC (permalink / raw)
To: Yajun Deng, davem, kuba; +Cc: netdev, linux-kernel
On 8/6/21 8:38 AM, Yajun Deng wrote:
> Add the case if sk is NULL in sock_{put, hold},
> The caller is free to use it.
>
Can we please stop adding code like that all over the places ?
This is wrong, fix the callers that are lazy, or fix the real bug
if this is a syzbot report.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: sock: add the case if sk is NULL
2021-08-09 6:12 ` yajun.deng
@ 2021-08-09 9:34 ` Eric Dumazet
2021-08-09 10:28 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2021-08-09 9:34 UTC (permalink / raw)
To: yajun.deng, Jakub Kicinski; +Cc: davem, netdev, linux-kernel
On 8/9/21 8:12 AM, yajun.deng@linux.dev wrote:
> August 6, 2021 9:11 PM, "Jakub Kicinski" <kuba@kernel.org> wrote:
>
>> On Fri, 6 Aug 2021 14:38:15 +0800 Yajun Deng wrote:
>>
>>> Add the case if sk is NULL in sock_{put, hold},
>>> The caller is free to use it.
>>>
>>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
>>
>> 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.
>> 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)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: sock: add the case if sk is NULL
2021-08-09 9:34 ` Eric Dumazet
@ 2021-08-09 10:28 ` Leon Romanovsky
0 siblings, 0 replies; 6+ messages in thread
From: Leon Romanovsky @ 2021-08-09 10:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: yajun.deng, Jakub Kicinski, davem, netdev, linux-kernel
On Mon, Aug 09, 2021 at 11:34:31AM +0200, Eric Dumazet wrote:
>
>
> On 8/9/21 8:12 AM, yajun.deng@linux.dev wrote:
> > August 6, 2021 9:11 PM, "Jakub Kicinski" <kuba@kernel.org> wrote:
> >
> >> On Fri, 6 Aug 2021 14:38:15 +0800 Yajun Deng wrote:
> >>
> >>> Add the case if sk is NULL in sock_{put, hold},
> >>> The caller is free to use it.
> >>>
> >>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> >>
> >> 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.
> >> 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)
You can add into your list that such "if NULL" checks add false
impression that NULL can be there and it is valid.
Thanks
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-09 10:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-06 6:38 [PATCH net-next] net: sock: add the case if sk is NULL Yajun Deng
2021-08-06 13:11 ` Jakub Kicinski
2021-08-09 6:12 ` yajun.deng
2021-08-09 9:34 ` Eric Dumazet
2021-08-09 10:28 ` Leon Romanovsky
2021-08-09 9:15 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).