Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Leon Romanovsky <leon@kernel.org>
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: yajun.deng@linux.dev, Jakub Kicinski <kuba@kernel.org>,
	davem@davemloft.net, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] net: sock: add the case if sk is NULL
Date: Mon, 9 Aug 2021 13:28:02 +0300	[thread overview]
Message-ID: <YREDMtIJ2Mz/ELy7@unreal> (raw)
In-Reply-To: <79e7c9a8-526c-ae9c-8273-d1d4d6170b69@gmail.com>

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

> 

  reply	other threads:[~2021-08-09 10:28 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-06  6:38 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 [this message]
2021-08-09  9:15 ` Eric Dumazet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YREDMtIJ2Mz/ELy7@unreal \
    --to=leon@kernel.org \
    --cc=davem@davemloft.net \
    --cc=eric.dumazet@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=yajun.deng@linux.dev \
    --subject='Re: [PATCH net-next] net: sock: add the case if sk is NULL' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).