linux-kernel - Re: [PATCH 4/9] drivers: Switch to kvfree

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] [day] [month] [year] [list]

Date:   Wed, 8 Dec 2021 11:24:35 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     "Uladzislau Rezki (Sony)" <urezki@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, RCU <rcu@...r.kernel.org>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Michal Hocko <mhocko@...e.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Daniel Axtens <dja@...ens.net>,
        Frederic Weisbecker <frederic@...nel.org>,
        Neeraj Upadhyay <neeraju@...eaurora.org>,
        Joel Fernandes <joel@...lfernandes.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Theodore Y . Ts'o" <tytso@....edu>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Oleksiy Avramchenko <oleksiy.avramchenko@...ymobile.com>,
        Philipp Reisner <philipp.reisner@...bit.com>,
        "Md. Haris Iqbal" <haris.iqbal@...os.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Mike Marciniszyn <mike.marciniszyn@...nelisnetworks.com>,
        Samuel Iglesias Gonsalvez <siglesias@...lia.com>,
        Lee Jones <lee.jones@...aro.org>,
        Jorgen Hansen <jhansen@...are.com>,
        Raju Rangoju <rajur@...lsio.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Boris Pismenny <borisp@...dia.com>,
        Jiri Pirko <jiri@...dia.com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH 4/9] drivers: Switch to kvfree_rcu() API

On Wed, Nov 24, 2021 at 12:03:03PM +0100, Uladzislau Rezki (Sony) wrote:
> Instead of invoking a synchronize_rcu() to free a pointer
> after a grace period we can directly make use of new API
> that does the same but in more efficient way.

It isn't entirely new, kfree_rcu() has been around for ages and any of
these call sites could have made use of it if they wanted. The
kvfree_rcu() just adds the twist of transparently allocating memory.

We really must ask in each case why the original author didn't use
kfree_rcu()..

>  drivers/block/drbd/drbd_nl.c                       | 9 +++------
>  drivers/block/drbd/drbd_receiver.c                 | 6 ++----
>  drivers/block/drbd/drbd_state.c                    | 3 +--
>  drivers/block/rnbd/rnbd-srv.c                      | 3 +--
>  drivers/crypto/nx/nx-common-pseries.c              | 3 +--
>  drivers/infiniband/hw/hfi1/sdma.c                  | 3 +--
>  drivers/ipack/carriers/tpci200.c                   | 3 +--
>  drivers/mfd/dln2.c                                 | 6 ++----
>  drivers/misc/vmw_vmci/vmci_context.c               | 6 ++----
>  drivers/misc/vmw_vmci/vmci_event.c                 | 3 +--
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c    | 3 +--
>  drivers/net/ethernet/mellanox/mlx5/core/en/qos.c   | 3 +--
>  drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 3 +--
>  drivers/net/ethernet/mellanox/mlxsw/core.c         | 3 +--
>  drivers/scsi/device_handler/scsi_dh_alua.c         | 3 +--
>  drivers/scsi/device_handler/scsi_dh_rdac.c         | 3 +--
>  drivers/staging/fwserial/fwserial.c                | 3 +--

These all need to be split to single patches and ack'ed by experts.

> diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
> index 44ccf8b4f4b2..28f4d84945fd 100644
> +++ b/drivers/block/drbd/drbd_nl.c
> @@ -1679,8 +1679,7 @@ int drbd_adm_disk_opts(struct sk_buff *skb, struct genl_info *info)
>  			drbd_send_sync_param(peer_device);
>  	}
>  
> -	synchronize_rcu();
> -	kfree(old_disk_conf);
> +	kvfree_rcu(old_disk_conf);
>  	kfree(old_plan);

For instance, this, how do you know that old_plan isn't also RCU
protected?

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> index dde1cf51d0ab..0619cb94f0e0 100644
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> @@ -7190,8 +7190,7 @@ static void remove_one(struct pci_dev *pdev)
>  	}
>  	pci_release_regions(pdev);
>  	kfree(adapter->mbox_log);
> -	synchronize_rcu();
> -	kfree(adapter);
> +	kvfree_rcu(adapter);
>  }

And this, for instance, just looks crazy! There is only one RCU region
in this file and it is not protecting an adaptor pointer, it is
protecting a netdev. No idea what this is trying to do today even.

Each case needs to be audited to make sure the synchronize_rcu() is
only protecting the kfree and not other things as well. It is tricky
stuff.

I see you got an Ack for the infiniband peice, so feel free to send
that file to the linux-rdma list.

Jason