netdev - Re: [PATCH v1 net-next] af_unix: Try not to hold unix_gc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 12 Apr 2024 19:05:22 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Kuniyuki Iwashima
 <kuni1840@...il.com>, <netdev@...r.kernel.org>, kernel test robot
 <oliver.sang@...el.com>
Subject: Re: [PATCH v1 net-next] af_unix: Try not to hold unix_gc_lock
 during accept().

On Wed, 10 Apr 2024 13:19:29 -0700 Kuniyuki Iwashima wrote:
>  void unix_update_edges(struct unix_sock *receiver)
>  {
> -	spin_lock(&unix_gc_lock);
> -	unix_update_graph(unix_sk(receiver->listener)->vertex);
> +	/* nr_unix_fds is only updated under unix_state_lock().
> +	 * If it's 0 here, the embryo socket is not part of the
> +	 * inflight graph, and GC will not see it.
> +	 */
> +	bool need_lock = !!receiver->scm_stat.nr_unix_fds;
> +
> +	if (need_lock) {
> +		spin_lock(&unix_gc_lock);
> +		unix_update_graph(unix_sk(receiver->listener)->vertex);
> +	}
> +
>  	receiver->listener = NULL;
> -	spin_unlock(&unix_gc_lock);
> +
> +	if (need_lock)
> +		spin_unlock(&unix_gc_lock);
>  }

Are you planning to add more code here? I feel like the sharing of 
a single line is outweighted by the conditionals.. I mean:

	/* ...
	 */
	if (!receiver->scm_stat.nr_unix_fd) {
		receiver->listener = NULL;
	} else {
		spin_lock(&unix_gc_lock);
		unix_update_graph(unix_sk(receiver->listener)->vertex);
		receiver->listener = NULL;
		spin_unlock(&unix_gc_lock);
	}

no?