Messages in this thread | | | From | Marco Elver <> | Date | Fri, 6 Dec 2019 14:14:43 +0100 | Subject | Re: [PATCH] kcov: fix struct layout for kcov_remote_arg |
| |
On Fri, 6 Dec 2019 at 14:05, Andrey Konovalov <andreyknvl@google.com> wrote: > > Make the layout of kcov_remote_arg the same for 32-bit and 64-bit code. > This makes it more convenient to write userspace apps that can be compiled > into 32-bit or 64-bit binaries and still work with the same 64-bit kernel. > Also use proper __u32 types in uapi headers instead of unsigned ints. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > --- > > Hi Andrew, > > We've noticed failures on 32 bit syzbot instances when the kcov patches > got merged into mainline. The reason is that the layout of kcov_remote_arg > depends on the alignment rules, which are different for 32/64 bit code. > We can deal with this issue in syzkaller [1], but I think it would be > cleander to get this fixed in the kernel. > > I hope this patch is acceptable, since the change has just been merged and > is not included into a release kernel version. The patch breaks the newly > introduced kcov API for 32 bit apps. > > Sorry for not testing it with 32 bit code earlier. > > Thanks! > > [1] https://github.com/google/syzkaller/commit/ba97c611a36b7729d489ebca5f97183c2ba7a90a > > Documentation/dev-tools/kcov.rst | 7 ++++--- > include/uapi/linux/kcov.h | 11 ++++++----- > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst > index 36890b026e77..744df2bae1ed 100644 > --- a/Documentation/dev-tools/kcov.rst > +++ b/Documentation/dev-tools/kcov.rst > @@ -251,9 +251,10 @@ selectively from different subsystems. > .. code-block:: c > > struct kcov_remote_arg { > - unsigned trace_mode; > - unsigned area_size; > - unsigned num_handles; > + uint32_t trace_mode; > + uint32_t area_size; > + uint32_t num_handles; > + uint32_t reserved; > uint64_t common_handle; > uint64_t handles[0]; > }; > diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h > index 409d3ad1e6e2..53267f9f1665 100644 > --- a/include/uapi/linux/kcov.h > +++ b/include/uapi/linux/kcov.h > @@ -9,11 +9,12 @@ > * and the comment before kcov_remote_start() for usage details. > */ > struct kcov_remote_arg { > - unsigned int trace_mode; /* KCOV_TRACE_PC or KCOV_TRACE_CMP */ > - unsigned int area_size; /* Length of coverage buffer in words */ > - unsigned int num_handles; /* Size of handles array */ > - __u64 common_handle; > - __u64 handles[0]; > + __u32 trace_mode; /* KCOV_TRACE_PC or KCOV_TRACE_CMP */ > + __u32 area_size; /* Length of coverage buffer in words */ > + __u32 num_handles; /* Size of handles array */ > + __u32 reserved;
The kernel provides __aligned_u64 for this purpose (32/64 bit uapi compatibility). Is adding an explicit 'reserved' better here? If so, it'd be good to document.
Thanks, -- Marco
| |