| Message ID | 20231121220155.1217090-20-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | kmsan: Enable on s390 | expand |
On Tue, Nov 21, 2023 at 11:07 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > The constraints of the DFLTCC inline assembly are not precise: they > do not communicate the size of the output buffers to the compiler, so > it cannot automatically instrument it. KMSAN usually does a poor job instrumenting inline assembly. Wouldn't be it better to switch to pure C ZLIB implementation, making ZLIB_DFLTCC depend on !KMSAN?
On Fri, 2023-12-08 at 14:32 +0100, Alexander Potapenko wrote: > On Tue, Nov 21, 2023 at 11:07 PM Ilya Leoshkevich <iii@linux.ibm.com> > wrote: > > > > The constraints of the DFLTCC inline assembly are not precise: they > > do not communicate the size of the output buffers to the compiler, > > so > > it cannot automatically instrument it. > > KMSAN usually does a poor job instrumenting inline assembly. > Wouldn't be it better to switch to pure C ZLIB implementation, making > ZLIB_DFLTCC depend on !KMSAN? Normally I would agree, but the kernel DFLTCC code base is synced with the zlib-ng code base to the extent that it uses the zlib-ng code style instead of the kernel code style, and MSAN annotations are already a part of the zlib-ng code base. So I would prefer to keep them for consistency. The code is also somewhat tricky in the are of buffer management, so I find it beneficial to have it checked for uninitialized memory accesses.
On Fri, Dec 8, 2023 at 3:14 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > On Fri, 2023-12-08 at 14:32 +0100, Alexander Potapenko wrote: > > On Tue, Nov 21, 2023 at 11:07 PM Ilya Leoshkevich <iii@linux.ibm.com> > > wrote: > > > > > > The constraints of the DFLTCC inline assembly are not precise: they > > > do not communicate the size of the output buffers to the compiler, > > > so > > > it cannot automatically instrument it. > > > > KMSAN usually does a poor job instrumenting inline assembly. > > Wouldn't be it better to switch to pure C ZLIB implementation, making > > ZLIB_DFLTCC depend on !KMSAN? > > Normally I would agree, but the kernel DFLTCC code base is synced with > the zlib-ng code base to the extent that it uses the zlib-ng code style > instead of the kernel code style, and MSAN annotations are already a > part of the zlib-ng code base. So I would prefer to keep them for > consistency. Hm, I didn't realize this code is being taken from elsewhere. If so, maybe we should come up with an annotation that can be contributed to zlib-ng, so that it doesn't cause merge conflicts every time Mikhail is doing an update? (leaving this up to you to decide). If you decide to go with the current solution, please consider adding an #include for kmsan-checks.h, which introduces kmsan_unpoison_memory().
diff --git a/lib/zlib_dfltcc/dfltcc.h b/lib/zlib_dfltcc/dfltcc.h index b96232bdd44d..0f2a16d7a48a 100644 --- a/lib/zlib_dfltcc/dfltcc.h +++ b/lib/zlib_dfltcc/dfltcc.h @@ -80,6 +80,7 @@ struct dfltcc_param_v0 { uint8_t csb[1152]; }; +static_assert(offsetof(struct dfltcc_param_v0, csb) == 384); static_assert(sizeof(struct dfltcc_param_v0) == 1536); #define CVT_CRC32 0 diff --git a/lib/zlib_dfltcc/dfltcc_util.h b/lib/zlib_dfltcc/dfltcc_util.h index 4a46b5009f0d..ce2e039a55b5 100644 --- a/lib/zlib_dfltcc/dfltcc_util.h +++ b/lib/zlib_dfltcc/dfltcc_util.h @@ -2,6 +2,7 @@ #ifndef DFLTCC_UTIL_H #define DFLTCC_UTIL_H +#include "dfltcc.h" #include <linux/zutil.h> /* @@ -20,6 +21,7 @@ typedef enum { #define DFLTCC_CMPR 2 #define DFLTCC_XPND 4 #define HBT_CIRCULAR (1 << 7) +#define DFLTCC_FN_MASK ((1 << 7) - 1) #define HB_BITS 15 #define HB_SIZE (1 << HB_BITS) @@ -34,6 +36,7 @@ static inline dfltcc_cc dfltcc( ) { Byte *t2 = op1 ? *op1 : NULL; + unsigned char *orig_t2 = t2; size_t t3 = len1 ? *len1 : 0; const Byte *t4 = op2 ? *op2 : NULL; size_t t5 = len2 ? *len2 : 0; @@ -59,6 +62,26 @@ static inline dfltcc_cc dfltcc( : "cc", "memory"); t2 = r2; t3 = r3; t4 = r4; t5 = r5; + switch (fn & DFLTCC_FN_MASK) { + case DFLTCC_QAF: + kmsan_unpoison_memory(param, sizeof(struct dfltcc_qaf_param)); + break; + case DFLTCC_GDHT: + kmsan_unpoison_memory(param, offsetof(struct dfltcc_param_v0, csb)); + break; + case DFLTCC_CMPR: + kmsan_unpoison_memory(param, sizeof(struct dfltcc_param_v0)); + kmsan_unpoison_memory( + orig_t2, + t2 - orig_t2 + + (((struct dfltcc_param_v0 *)param)->sbb == 0 ? 0 : 1)); + break; + case DFLTCC_XPND: + kmsan_unpoison_memory(param, sizeof(struct dfltcc_param_v0)); + kmsan_unpoison_memory(orig_t2, t2 - orig_t2); + break; + } + if (op1) *op1 = t2; if (len1)
The constraints of the DFLTCC inline assembly are not precise: they do not communicate the size of the output buffers to the compiler, so it cannot automatically instrument it. Add the manual kmsan_unpoison_memory() calls for the output buffers. The logic is the same as in [1]. [1] https://github.com/zlib-ng/zlib-ng/commit/1f5ddcc009ac3511e99fc88736a9e1a6381168c5 Reported-by: Alexander Gordeev <agordeev@linux.ibm.com> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- lib/zlib_dfltcc/dfltcc.h | 1 + lib/zlib_dfltcc/dfltcc_util.h | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+)