netdev - Re: [PATCH v3 net-next 13/14] af_unix: Replace garbage collection algorithm.

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]

Message-ID: <2e6eb578782dffcc8481992ba39181c74e2a7f80.camel@redhat.com>
Date: Tue, 27 Feb 2024 12:36:51 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>, "David S. Miller"
	 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	 <kuba@...nel.org>
Cc: Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 13/14] af_unix: Replace garbage collection
 algorithm.

On Fri, 2024-02-23 at 13:40 -0800, Kuniyuki Iwashima wrote:
> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> index 060e81be3614..59a87a997a4d 100644
> --- a/net/unix/garbage.c
> +++ b/net/unix/garbage.c
> @@ -314,6 +314,48 @@ static bool unix_vertex_dead(struct unix_vertex *vertex)
>  	return true;
>  }
>  
> +static struct sk_buff_head hitlist;

I *think* hitlist could be replaced with a local variable in
__unix_gc(), WDYT?

> +
> +static void unix_collect_skb(struct list_head *scc)
> +{
> +	struct unix_vertex *vertex;
> +
> +	list_for_each_entry_reverse(vertex, scc, scc_entry) {
> +		struct sk_buff_head *queue;
> +		struct unix_edge *edge;
> +		struct unix_sock *u;
> +
> +		edge = list_first_entry(&vertex->edges, typeof(*edge), vertex_entry);
> +		u = edge->predecessor;
> +		queue = &u->sk.sk_receive_queue;
> +
> +		spin_lock(&queue->lock);
> +
> +		if (u->sk.sk_state == TCP_LISTEN) {
> +			struct sk_buff *skb;
> +
> +			skb_queue_walk(queue, skb) {
> +				struct sk_buff_head *embryo_queue = &skb->sk->sk_receive_queue;
> +
> +				spin_lock(&embryo_queue->lock);

I'm wondering if and why lockdep would be happy about the above. I
think this deserve at least a comment.


> +				skb_queue_splice_init(embryo_queue, &hitlist);
> +				spin_unlock(&embryo_queue->lock);
> +			}
> +		} else {
> +			skb_queue_splice_init(queue, &hitlist);
> +
> +#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
> +			if (u->oob_skb) {
> +				kfree_skb(u->oob_skb);

Similar question here. This happens under the u receive queue lock,
could we his some complex lock dependency? what about moving oob_skb to
hitlist instead?


Cheers,

Paolo