netdev - Re: [PATCH net-next] can: gw: use call_rcu() instead of costly synchronize

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: <43ec78a3-2cde-35c4-51e9-de72d00fbe99@hartkopp.net>
Date:   Tue, 8 Feb 2022 20:27:31 +0100
From:   Oliver Hartkopp <socketcan@...tkopp.net>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     netdev <netdev@...r.kernel.org>, Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next] can: gw: use call_rcu() instead of costly
 synchronize_rcu()



On 07.02.22 20:07, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@...gle.com>
> 
> Commit fb8696ab14ad ("can: gw: synchronize rcu operations
> before removing gw job entry") added three synchronize_rcu() calls
> to make sure one rcu grace period was observed before freeing
> a "struct cgw_job" (which are tiny objects).
> 
> This should be converted to call_rcu() to avoid adding delays
> in device / network dismantles.
> 
> Use the rcu_head that was already in struct cgw_job,
> not yet used.
> 
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Oliver Hartkopp <socketcan@...tkopp.net>

Tested-by: Oliver Hartkopp <socketcan@...tkopp.net>

Thanks Eric!

> ---
>   net/can/gw.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index d8861e862f157aec36c417b71eb7e8f59bd064b9..20e74fe7d0906dccc65732b8f9e7e14e2d1192c3 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -577,6 +577,13 @@ static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
>   			  gwj->ccgw.filter.can_mask, can_can_gw_rcv, gwj);
>   }
>   
> +static void cgw_job_free_rcu(struct rcu_head *rcu_head)
> +{
> +	struct cgw_job *gwj = container_of(rcu_head, struct cgw_job, rcu);
> +
> +	kmem_cache_free(cgw_cache, gwj);
> +}
> +
>   static int cgw_notifier(struct notifier_block *nb,
>   			unsigned long msg, void *ptr)
>   {
> @@ -596,8 +603,7 @@ static int cgw_notifier(struct notifier_block *nb,
>   			if (gwj->src.dev == dev || gwj->dst.dev == dev) {
>   				hlist_del(&gwj->list);
>   				cgw_unregister_filter(net, gwj);
> -				synchronize_rcu();
> -				kmem_cache_free(cgw_cache, gwj);
> +				call_rcu(&gwj->rcu, cgw_job_free_rcu);
>   			}
>   		}
>   	}
> @@ -1155,8 +1161,7 @@ static void cgw_remove_all_jobs(struct net *net)
>   	hlist_for_each_entry_safe(gwj, nx, &net->can.cgw_list, list) {
>   		hlist_del(&gwj->list);
>   		cgw_unregister_filter(net, gwj);
> -		synchronize_rcu();
> -		kmem_cache_free(cgw_cache, gwj);
> +		call_rcu(&gwj->rcu, cgw_job_free_rcu);
>   	}
>   }
>   
> @@ -1224,8 +1229,7 @@ static int cgw_remove_job(struct sk_buff *skb, struct nlmsghdr *nlh,
>   
>   		hlist_del(&gwj->list);
>   		cgw_unregister_filter(net, gwj);
> -		synchronize_rcu();
> -		kmem_cache_free(cgw_cache, gwj);
> +		call_rcu(&gwj->rcu, cgw_job_free_rcu);
>   		err = 0;
>   		break;
>   	}