Hi Nitin,
On 5/23/07, Nitin Gupta <[email protected]> wrote:
[...]
diff --git a/Makefile b/Makefile
index 34210af..88053ba 100644
--- a/Makefile
+++ b/Makefile
@@ -826,11 +826,18 @@ include/config/kernel.release:
include/config/auto.conf FORCE
# Listed in dependency order
PHONY += prepare archprepare prepare0 prepare1 prepare2 prepare3
+# prepare4 does module specific things prior to compilation
+prepare4:
+ifneq ($(CONFIG_LZO1X),n)
+ $(Q)ln -sf $(srctree)/lib/lzo1x/lzo1x_decompress.c \
+ $(srctree)/lib/lzo1x/lzo1x_decompress_safe.c
+endif
+
Is this addition to the master Makefile really necessary? We're just
setting up a source file link here ... is this something temporary that
you're proposing that'll last only till this gets tested or do you want
this kind of special-casing for lzo1x here permanently?
diff --git a/include/linux/lzo1x.h b/include/linux/lzo1x.h
[...]
+/* Size of temp buffer (workmem) required by lzo1x_compress */
+#define LZO1X_WORKMEM_SIZE ((size_t) (16384L * sizeof(unsigned char *)))
+
+/*
+ * This required 'workmem' of size LZO1X_WORKMEM_SIZE
+ */
+int lzo1x_compress(const unsigned char *src, size_t src_len,
+ unsigned char *dst, size_t *dst_len,
+ void *workmem);
Just defining and exporting LZO1X_WORKMEM_SIZE may not be enough
to guarantee that users _will_ pass in workmem of size exactly that much.
If this workmem is really merely a temp buffer required by lzo1x_compress(),
I'd suggest you rename lzo1x_compress() in lzo1x_compress.c to
__lzo1x_compress(), and implement a lzo1x_compress() wrapper for the
user that handles the allocation (and subsequent free'ing) of this temp
buffer itself.
[ I vaguely remember discussing something of this sort with Richard
when he had submitted his patchset too, perhaps you can look into
his implementation to see how he's managing this ... ]
+/*
+ * This decompressor expects valid compressed data.
+ *
+ * If the compressed data gets corrupted somehow (e.g. transmission
+ * via an erroneous channel, disk errors, ...) it will probably crash
+ * your application because absolutely no additional checks are done.
^^^^^^^^^^^
Whoa! "your application" here is _kernel code_ and not a userspace
program ... "crashing" it is something we could do without :-)
+ */
+int lzo1x_decompress(const unsigned char *src, size_t src_len,
+ unsigned char *dst, size_t *dst_len);
+
+
+/*
+ * The `safe' decompressor. Somewhat slower.
+ *
+ * This decompressor will catch all compressed data violations and
+ * return an error code in this case.
+ */
+int lzo1x_decompress_safe(const unsigned char *src, size_t src_len,
+ unsigned char *dst, size_t *dst_len);
+#endif
I just read the follow-ups to this, so perhaps we /can/ use the unsafe
versions in certain situations. But I agree with Michael's suggestion
to rename _safe to decompress and decompress to _unsafe ...
diff --git a/lib/lzo1x/Makefile b/lib/lzo1x/Makefile
[...]
+#
+# When compiling this module out of tree, do 'make prepare_lzo'
+# before compiling as usual
+#
+obj-$(CONFIG_LZO1X) += lzo1x.o
+CFLAGS_lzo1x_decompress_safe.o += -DLZO1X_DECOMPRESS_SAFE
+lzo1x-objs := lzo1x_compress.o lzo1x_decompress.o lzo1x_decompress_safe.o
+
+prepare_lzo:
+ @ln -sf lzo1x_decompress.c lzo1x_decompress_safe.c
+
... ah, so that's why the master Makefile changes.
Hmmm, perhaps you could extract the common stuff between the
_safe and _unsafe versions out into a separate function and then
reuse it from _safe and _unsafe wrappers? In any case, this kind
of Makefile jugglery (even in the master Makefile) just to avoid the
above doesn't seem quite right ...
diff --git a/lib/lzo1x/lzo1x_decompress.c b/lib/lzo1x/lzo1x_decompress.c
[...]
+#if defined(LZO1X_DECOMPRESS_SAFE)
+input_overrun:
+ *out_len = (size_t)(op - out);
+ return LZO_E_INPUT_OVERRUN;
+
+output_overrun:
+ *out_len = (size_t)(op - out);
+ return LZO_E_OUTPUT_OVERRUN;
+
+lookbehind_overrun:
+ *out_len = (size_t)(op - out);
+ return LZO_E_LOOKBEHIND_OVERRUN;
+#endif
+}
+
+EXPORT_SYMBOL(lzo1x_decompress);
Ok, so this is all there is between _safe and _unsafe, it seems ...
diff --git a/lib/lzo1x/lzo1x_int.h b/lib/lzo1x/lzo1x_int.h
[...]
+/* Macros for 'safe' decompression */
+#ifdef LZO1X_DECOMPRESS_SAFE
+
+#define lzo1x_decompress lzo1x_decompress_safe
+#define TEST_IP (ip < ip_end)
+#define NEED_IP(x) \
+ if ((size_t)(ip_end - ip) < (size_t)(x)) goto input_overrun
+#define NEED_OP(x) \
+ if ((size_t)(op_end - op) < (size_t)(x)) goto output_overrun
+#define TEST_LB(m_pos) \
+ if (m_pos < out || m_pos >= op) goto lookbehind_overrun
+#define HAVE_TEST_IP
+#define HAVE_ANY_OP
+
+#else /* !LZO1X_DECOMPRESS_SAFE */
+
+#define TEST_IP 1
+#define TEST_LB(x) ((void) 0)
+#define NEED_IP(x) ((void) 0)
+#define NEED_OP(x) ((void) 0)
+#undef HAVE_TEST_IP
+#undef HAVE_ANY_OP
+
+#endif /* LZO1X_DECOMPRESS_SAFE */
... ugh. Yes, extracting the common stuff between the _safe and _unsafe
variants in a common low-level __lzo1x_decompress kind of function
definitely looks doable. The low-level function could simply take an extra
argument (say, set by the _safe and _unsafe wrappers) that tells it
whether it is being called as safe or unsafe ... helps us get rid of the
disruptions to all the Makefiles above and these #ifdef's ugliness ...
BTW, it'd be really cool if Richard and yourself could get together and
pool your energies / efforts to develop a common / same patchset for this.
(I wonder how different your implementations are, actually, and if there
are any significant performance disparities, especially.) I really like your
work, as it clears up the major gripe I had with Richard's patchset -- the
ugliness (and monstrosity) of it. But he's also worked up the glue code for
cryptoapi / jffs2 etc for this, so no point duplicating his efforts.
Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
