netdev - Re: [PATCH] can: gw: prefer kfree_rcu() over call_rcu() with cgw_job_free

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: <CANn89iLCK10J_6=1xSDquYpToZ-YNG3TzjS60L-g5Cyng92gFw@mail.gmail.com>
Date: Wed, 13 Mar 2024 11:18:19 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Dmitry Antipov <dmantipov@...dex.ru>
Cc: Oliver Hartkopp <socketcan@...tkopp.net>, Marc Kleine-Budde <mkl@...gutronix.de>, 
	linux-can@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] can: gw: prefer kfree_rcu() over call_rcu() with cgw_job_free_rcu()

On Wed, Mar 13, 2024 at 10:43 AM Dmitry Antipov <dmantipov@...dex.ru> wrote:
>
> Drop trivial free-only 'cgw_job_free_rcu()' RCU callback and
> switch to 'kfree_rcu()' in 'cgw_notifier()', 'cgw_remove_job()'
> and 'cgw_remove_all_jobs()'. Compile tested only.
>
> Signed-off-by: Dmitry Antipov <dmantipov@...dex.ru>
> ---
>  net/can/gw.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/net/can/gw.c b/net/can/gw.c
> index 37528826935e..ffb9870e2d01 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -577,13 +577,6 @@ 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);

kmem_cache_free() is not the same than kfree()

Unless I have missed something in mm territory , your patch is not
going to work.

> -}
> -
>  static int cgw_notifier(struct notifier_block *nb,
>                         unsigned long msg, void *ptr)
>  {
> @@ -603,7 +596,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);
> -                               call_rcu(&gwj->rcu, cgw_job_free_rcu);
> +                               kfree_rcu(gwj, rcu);
>                         }
>                 }
>         }
> @@ -1168,7 +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);
> -               call_rcu(&gwj->rcu, cgw_job_free_rcu);
> +               kfree_rcu(gwj, rcu);
>         }
>  }
>
> @@ -1236,7 +1229,7 @@ static int cgw_remove_job(struct sk_buff *skb, struct nlmsghdr *nlh,
>
>                 hlist_del(&gwj->list);
>                 cgw_unregister_filter(net, gwj);
> -               call_rcu(&gwj->rcu, cgw_job_free_rcu);
> +               kfree_rcu(gwj, rcu);
>                 err = 0;
>                 break;
>         }
> --
> 2.44.0
>