[PATCH] nvme: Fix nvme_setup_command metadata trace event for cdw10

[PATCH] nvme: Fix nvme_setup_command metadata trace event for cdw10

[John Garry]

For x86_64 allmodconfig I get this warning:

In function ‘fortify_memcpy_chk’,
    inlined from ‘perf_trace_nvme_setup_cmd’ at drivers/nvme/host/./trace.h:47:1:
./include/linux/fortify-string.h:352:4: error: call to ‘__read_overflow2_field’ declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror]
    __read_overflow2_field(q_size_field, size);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘fortify_memcpy_chk’,
    inlined from ‘trace_event_raw_event_nvme_setup_cmd’ at drivers/nvme/host/./trace.h:47:1:
./include/linux/fortify-string.h:352:4: error: call to ‘__read_overflow2_field’ declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror]
    __read_overflow2_field(q_size_field, size);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

cdw10 metadata is 24 bytes, and we try to copy size of cdw10 metadata from
nvme_command.common.cdw10 into that cdw10 metadata, but
nvme_command.common.cdw10 is only 4 bytes in size.

Fix by making the trace metadata size as 4 bytes.

I find that this warning started first appearing from commit f68f2ff91512
("fortify: Detect struct member overflows in memcpy() at compile-time").

Signed-off-by: John Garry <john.garry@huawei.com>

diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
index b5f85259461a..d6d35f935006 100644
--- a/drivers/nvme/host/trace.h
+++ b/drivers/nvme/host/trace.h
@@ -57,7 +57,7 @@ TRACE_EVENT(nvme_setup_cmd,
 		__field(u16, cid)
 		__field(u32, nsid)
 		__field(bool, metadata)
-		__array(u8, cdw10, 24)
+		__array(u8, cdw10, 4)
 	    ),
 	    TP_fast_assign(
 		__entry->ctrl_id = nvme_req(req)->ctrl->instance;
-- 
2.35.3

Re: [PATCH] nvme: Fix nvme_setup_command metadata trace event for cdw10

[Christoph Hellwig]

Thanks,

applied to nvme-5.19.

Re: [PATCH] nvme: Fix nvme_setup_command metadata trace event for cdw10

[Keith Busch]

On Wed, Jul 06, 2022 at 04:16:38PM +0800, John Garry wrote:
> For x86_64 allmodconfig I get this warning:
> 
> In function ‘fortify_memcpy_chk’,
>     inlined from ‘perf_trace_nvme_setup_cmd’ at drivers/nvme/host/./trace.h:47:1:
> ./include/linux/fortify-string.h:352:4: error: call to ‘__read_overflow2_field’ declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror]
>     __read_overflow2_field(q_size_field, size);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In function ‘fortify_memcpy_chk’,
>     inlined from ‘trace_event_raw_event_nvme_setup_cmd’ at drivers/nvme/host/./trace.h:47:1:
> ./include/linux/fortify-string.h:352:4: error: call to ‘__read_overflow2_field’ declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror]
>     __read_overflow2_field(q_size_field, size);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> cdw10 metadata is 24 bytes, and we try to copy size of cdw10 metadata from
> nvme_command.common.cdw10 into that cdw10 metadata, but
> nvme_command.common.cdw10 is only 4 bytes in size.
> 
> Fix by making the trace metadata size as 4 bytes.
> 
> I find that this warning started first appearing from commit f68f2ff91512
> ("fortify: Detect struct member overflows in memcpy() at compile-time").

Did you test what the trace looks like afte this? We're losing valuable trace
data here. The field is supposed to get CDW's 10 - 15, so that's 24 bytes. I
don't know why it cares that the address of the field being read is only 4
bytes; we want everything that comes after it too.

Re: [PATCH] nvme: Fix nvme_setup_command metadata trace event for cdw10

[Christoph Hellwig]

On Wed, Jul 06, 2022 at 10:13:22AM -0600, Keith Busch wrote:
> Did you test what the trace looks like afte this? We're losing valuable trace
> data here. The field is supposed to get CDW's 10 - 15, so that's 24 bytes. I
> don't know why it cares that the address of the field being read is only 4
> bytes; we want everything that comes after it too.

Because accesses should not spawn boundaries of members in structs unless
copying the entire struct.  If we want to trace the various fields we
need to individually assign them.

Anyway, I'm dropping this patch from nvme-5.19 for now to let the
discussion conclude.

Re: [PATCH] nvme: Fix nvme_setup_command metadata trace event for cdw10

[Keith Busch]

On Wed, Jul 06, 2022 at 06:18:25PM +0200, Christoph Hellwig wrote:
> On Wed, Jul 06, 2022 at 10:13:22AM -0600, Keith Busch wrote:
> > Did you test what the trace looks like afte this? We're losing valuable trace
> > data here. The field is supposed to get CDW's 10 - 15, so that's 24 bytes. I
> > don't know why it cares that the address of the field being read is only 4
> > bytes; we want everything that comes after it too.
> 
> Because accesses should not spawn boundaries of members in structs unless
> copying the entire struct.  If we want to trace the various fields we
> need to individually assign them.
> 
> Anyway, I'm dropping this patch from nvme-5.19 for now to let the
> discussion conclude.

How about this instead?

---
diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
index b5f85259461a..3c5e7fa03707 100644
--- a/drivers/nvme/host/trace.h
+++ b/drivers/nvme/host/trace.h
@@ -69,7 +69,7 @@ TRACE_EVENT(nvme_setup_cmd,
 		__entry->metadata = !!blk_integrity_rq(req);
 		__entry->fctype = cmd->fabrics.fctype;
 		__assign_disk_name(__entry->disk, req->q->disk);
-		memcpy(__entry->cdw10, &cmd->common.cdw10,
+		memcpy(__entry->cdw10, &cmd->common.bytes,
 			sizeof(__entry->cdw10));
 	    ),
 	    TP_printk("nvme%d: %sqid=%d, cmdid=%u, nsid=%u, flags=0x%x, meta=0x%x, cmd=(%s %s)",
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index e3934003f239..1be226871763 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -906,12 +906,17 @@ struct nvme_common_command {
 	__le32			cdw2[2];
 	__le64			metadata;
 	union nvme_data_ptr	dptr;
-	__le32			cdw10;
-	__le32			cdw11;
-	__le32			cdw12;
-	__le32			cdw13;
-	__le32			cdw14;
-	__le32			cdw15;
+	union {
+		struct {
+			__le32	cdw10;
+			__le32	cdw11;
+			__le32	cdw12;
+			__le32	cdw13;
+			__le32	cdw14;
+			__le32	cdw15;
+		};
+		__u8 bytes[24];
+	};
 };
 
 struct nvme_rw_command {
--

Re: [PATCH] nvme: Fix nvme_setup_command metadata trace event for cdw10

[Christoph Hellwig]

On Wed, Jul 06, 2022 at 10:26:09AM -0600, Keith Busch wrote:
> On Wed, Jul 06, 2022 at 06:18:25PM +0200, Christoph Hellwig wrote:
> > On Wed, Jul 06, 2022 at 10:13:22AM -0600, Keith Busch wrote:
> > > Did you test what the trace looks like afte this? We're losing valuable trace
> > > data here. The field is supposed to get CDW's 10 - 15, so that's 24 bytes. I
> > > don't know why it cares that the address of the field being read is only 4
> > > bytes; we want everything that comes after it too.
> > 
> > Because accesses should not spawn boundaries of members in structs unless
> > copying the entire struct.  If we want to trace the various fields we
> > need to individually assign them.
> > 
> > Anyway, I'm dropping this patch from nvme-5.19 for now to let the
> > discussion conclude.
> 
> How about this instead?

Maybe a better option would be to use struct_group().

Re: [PATCH] nvme: Fix nvme_setup_command metadata trace event for cdw10

[Keith Busch]

On Wed, Jul 06, 2022 at 06:34:34PM +0200, Christoph Hellwig wrote:
> On Wed, Jul 06, 2022 at 10:26:09AM -0600, Keith Busch wrote:
> > On Wed, Jul 06, 2022 at 06:18:25PM +0200, Christoph Hellwig wrote:
> > > On Wed, Jul 06, 2022 at 10:13:22AM -0600, Keith Busch wrote:
> > > > Did you test what the trace looks like afte this? We're losing valuable trace
> > > > data here. The field is supposed to get CDW's 10 - 15, so that's 24 bytes. I
> > > > don't know why it cares that the address of the field being read is only 4
> > > > bytes; we want everything that comes after it too.
> > > 
> > > Because accesses should not spawn boundaries of members in structs unless
> > > copying the entire struct.  If we want to trace the various fields we
> > > need to individually assign them.
> > > 
> > > Anyway, I'm dropping this patch from nvme-5.19 for now to let the
> > > discussion conclude.
> > 
> > How about this instead?
> 
> Maybe a better option would be to use struct_group().

Good call, I'd never used that macro before. The result produces anonymous
unions like I just proposed, so yes, I like that option.

Re: [PATCH] nvme: Fix nvme_setup_command metadata trace event for cdw10

[John Garry]

On 06/07/2022 17:44, Keith Busch wrote:
> On Wed, Jul 06, 2022 at 06:34:34PM +0200, Christoph Hellwig wrote:
>> On Wed, Jul 06, 2022 at 10:26:09AM -0600, Keith Busch wrote:
>>> On Wed, Jul 06, 2022 at 06:18:25PM +0200, Christoph Hellwig wrote:
>>>> On Wed, Jul 06, 2022 at 10:13:22AM -0600, Keith Busch wrote:
>>>>> Did you test what the trace looks like afte this? We're losing valuable trace
>>>>> data here. The field is supposed to get CDW's 10 - 15, so that's 24 bytes.

ok, I just thought it was a typo, but did not know why you were using an 
array macro.

> I
>>>>> don't know why it cares that the address of the field being read is only 4
>>>>> bytes; we want everything that comes after it too.
>>>>
>>>> Because accesses should not spawn boundaries of members in structs unless
>>>> copying the entire struct.  If we want to trace the various fields we
>>>> need to individually assign them.
>>>>
>>>> Anyway, I'm dropping this patch from nvme-5.19 for now to let the
>>>> discussion conclude.
>>>
>>> How about this instead?
>>
>> Maybe a better option would be to use struct_group().
> 
> Good call, I'd never used that macro before. The result produces anonymous
> unions like I just proposed, so yes, I like that option.
> .

The warning hints at using struct_group() also ...

Anyway, Keith, do you want to write a new patch or shall I?

Thanks,
John