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 <[email protected]>
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
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().
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 {
--
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.
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.
Thanks,
applied to nvme-5.19.
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.
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