Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <5ec092d7-6a51-04cc-bd70-12462524fda0@roeck-us.net>
Date:   Fri, 9 Sep 2022 13:36:24 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART
 log-buffer
Content-Language: en-US
To:     Serge Semin <Sergey.Semin@baikalelectronics.ru>,
        Jonathan Derrick <jonathan.derrick@intel.com>,
        Revanth Rajashekar <revanth.rajashekar@intel.com>,
        Jens Axboe <axboe@kernel.dk>, Keith Busch <kbusch@kernel.org>,
        Jens Axboe <axboe@fb.com>, Christoph Hellwig <hch@lst.de>,
        Sagi Grimberg <sagi@grimberg.me>
Cc:     Serge Semin <fancer.lancer@gmail.com>,
        Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220909191916.16013-1-Sergey.Semin@baikalelectronics.ru>
 <20220909191916.16013-2-Sergey.Semin@baikalelectronics.ru>
From:   Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <20220909191916.16013-2-Sergey.Semin@baikalelectronics.ru>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/9/22 12:19, Serge Semin wrote:
> Recent commit 52fde2c07da6 ("nvme: set dma alignment to dword") has caused
> a regression on our platform. It turned out that the nvme_get_log() method
> invocation caused the nvme_hwmon_data structure instance corruption. In
> particular the nvme_hwmon_data.ctrl pointer was overwritten either with
> zeros or with garbage. After some researches we discovered that the
> problem happened even before the actual NVME DMA execution, but during the
> buffer mapping. Since our platform was DMA-noncoherent the mapping implied
> the cache-lines invalidations or write-backs depending on the
> DMA-direction parameter. In case of the NVME SMART log getting the DMA
> was performed from-device-to-memory, thus the cache-invalidation was
> activated during the buffer mapping. Since the log-buffer wasn't
> cache-line aligned the cache-invalidation caused the neighbour data
> discard. The neighbouring data turned to be the data surrounding the
> buffer in the framework of the nvme_hwmon_data structure.
> 
> In order to fix that we need to make sure that the whole log-buffer is
> defined within the cache-line-aligned memory region so the
> cache-invalidation procedure wouldn't involve the adjacent data. By doing
> so we not only get rid from the denoted problem but also fulfill the
> requirement explicitly described in [1].
> 
> After a deeper researches we found out that the denoted commit wasn't a
> root cause of the problem. It just revealed the invalidity by activating
> the DMA-based NVME SMART log getting performed in the framework of the
> NVME hwmon driver. The problem was here since the initial commit of the
> driver.
> 
> [1] Documentation/core-api/dma-api.rst
> 
> Fixes: 400b6a7b13a3 ("nvme: Add hardware monitoring support")
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Thanks for tracking this down and for the fix.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> 
> ---
> 
> Folks, I've thoroughly studied the whole NVME subsystem looking for
> similar problems. Turned out there is one more place which may cause the
> same issue. It's connected with the opal_dev.{cmd,req} buffers passed to
> the nvme_sec_submit() method. The rest of the buffers involved in the NVME
> DMA are either allocated by kmalloc (must be cache-line-aligned by design)
> or bounced-buffered if allocated on the stack (see the blk_rq_map_kern()
> method implementation).
> ---
>   drivers/nvme/host/hwmon.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
> index 0a586d712920..94192ab7a02d 100644
> --- a/drivers/nvme/host/hwmon.c
> +++ b/drivers/nvme/host/hwmon.c
> @@ -10,9 +10,10 @@
>   
>   #include "nvme.h"
>   
> +/* DMA-noncoherent platforms require the cache-aligned buffers */
>   struct nvme_hwmon_data {
> +	struct nvme_smart_log log ____cacheline_aligned;
>   	struct nvme_ctrl *ctrl;
> -	struct nvme_smart_log log;
>   	struct mutex read_lock;
>   };
>