Messages in this thread | | | From | Andrey Konovalov <> | Date | Fri, 6 Dec 2019 14:31:53 +0100 | Subject | Re: [PATCH] kcov: fix struct layout for kcov_remote_arg |
| |
On Fri, Dec 6, 2019 at 2:14 PM Marco Elver <elver@google.com> wrote: > > 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).
Oh yes, this is much better, sent v2, thank you!
> Is adding an explicit 'reserved' better here? If so, > it'd be good to document.
| |