The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). The LLVMLinux Project is working towards the ability of
providing the Linux kernel developer the choice of using the Clang compiler
toolchain. This is a part of a series of patches which remove the use of VLAIS
from crypto code, dm-crypt, jbd2, libcrc32c, netfilter, and usb gadget. Other
patches to allow Clang to be used will follow.
Mark Charlebois (1):
Remove VLAIS usage from JBD2 code
include/linux/jbd2.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
1.7.9.5
From: Mark Charlebois <[email protected]>
The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). Since ctx is always a 32-bit CRC, hard coding a size of 4
bytes accomplishes the same thing without the use of VLAIS. This is the same
technique already employed in fs/ext4/ext4.h
Signed-off-by: Mark Charlebois <[email protected]>
Signed-off-by: Behan Webster <[email protected]>
---
include/linux/jbd2.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 3efc43f..efcbdfc 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1308,7 +1308,7 @@ static inline u32 jbd2_chksum(journal_t *journal, u32 crc,
{
struct {
struct shash_desc shash;
- char ctx[crypto_shash_descsize(journal->j_chksum_driver)];
+ char ctx[4];
} desc;
int err;
--
1.7.9.5
On Tue, Oct 30, 2012 at 02:40:04PM -0400, Behan Webster wrote:
> From: Mark Charlebois <[email protected]>
>
> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> precludes the use of compilers which don't implement VLAIS (for instance the
> Clang compiler). Since ctx is always a 32-bit CRC, hard coding a size of 4
> bytes accomplishes the same thing without the use of VLAIS. This is the same
> technique already employed in fs/ext4/ext4.h
>
> Signed-off-by: Mark Charlebois <[email protected]>
> Signed-off-by: Behan Webster <[email protected]>
That's reasonable, but in order to be safe to make sure we don't
accidentally introduce a stack overrun bug at some point in the
future, we should do something like this instead
+ #define JBD_MAX_CHECKSUM_SIZE 4
.
.
.
- char ctx[crypto_shash_descsize(journal->j_chksum_driver)];
+ char ctx[JBD_MAX_CHECKSUM_SIZE];
.
.
.
+ BUG_ON(crypto_shash_descsize(journal->j_chksum_driver) >
+ JBD_MAX_CHECKSUM_SIZE);
I just like being careful and paranoid; using magic numeric constants
for buffer sizes is just a scary thing to do. If you could resubmit
the patch with this change, I'd really appreciate it. Thanks!!
- Ted
On 12-10-30 03:00 PM, Theodore Ts'o wrote:
> On Tue, Oct 30, 2012 at 02:40:04PM -0400, Behan Webster wrote:
>> From: Mark Charlebois <[email protected]>
>>
>> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
>> precludes the use of compilers which don't implement VLAIS (for instance the
>> Clang compiler). Since ctx is always a 32-bit CRC, hard coding a size of 4
>> bytes accomplishes the same thing without the use of VLAIS. This is the same
>> technique already employed in fs/ext4/ext4.h
>>
>> Signed-off-by: Mark Charlebois <[email protected]>
>> Signed-off-by: Behan Webster <[email protected]>
> That's reasonable, but in order to be safe to make sure we don't
> accidentally introduce a stack overrun bug at some point in the
> future, we should do something like this instead
>
> + #define JBD_MAX_CHECKSUM_SIZE 4
> .
> .
> .
>
> - char ctx[crypto_shash_descsize(journal->j_chksum_driver)];
> + char ctx[JBD_MAX_CHECKSUM_SIZE];
> .
> .
> .
> + BUG_ON(crypto_shash_descsize(journal->j_chksum_driver) >
> + JBD_MAX_CHECKSUM_SIZE);
>
>
> I just like being careful and paranoid; using magic numeric constants
> for buffer sizes is just a scary thing to do. If you could resubmit
> the patch with this change, I'd really appreciate it. Thanks!!
A very good idea. Will do. Expect it soon.
Behan
--
Behan Webster
[email protected]
On Tue, Oct 30, 2012 at 02:40:04PM -0400, Behan Webster wrote:
> From: Mark Charlebois <[email protected]>
>
> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> precludes the use of compilers which don't implement VLAIS (for instance the
> Clang compiler). Since ctx is always a 32-bit CRC, hard coding a size of 4
> bytes accomplishes the same thing without the use of VLAIS. This is the same
> technique already employed in fs/ext4/ext4.h
>
> Signed-off-by: Mark Charlebois <[email protected]>
> Signed-off-by: Behan Webster <[email protected]>
> ---
> include/linux/jbd2.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 3efc43f..efcbdfc 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1308,7 +1308,7 @@ static inline u32 jbd2_chksum(journal_t *journal, u32 crc,
> {
> struct {
> struct shash_desc shash;
> - char ctx[crypto_shash_descsize(journal->j_chksum_driver)];
> + char ctx[4];
I wonder if this code ought to have a defensive programming check such as
BUG_ON(crypto_shash_descsize(journal->j_chksum_driver) > 4) here just in case
we ever decide to support a checksum function that is bigger than 32 bits?
At this stage of the game I doubt we'll be adding larger checksums to
ext4/jbd2, but I wouldn't want to rely on remembering this detail if we ever
do want to change it. I guess we could hide the check behind CONFIG_JBD_DEBUG
if we're concerned about slowing down the hot path.
The same comment applies to Ted's patch ("ext4: remove dynamic array size in
ext4_chksum()") back in July.
<shrug> Otherwise,
Acked-by: Darrick J. Wong <[email protected]>
--D
> } desc;
> int err;
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey Darrick,
I was looking at this code a bit more closely while applying the
revised version of this patch --- and this in particular raised a red
flag for me:
*(u32 *)desc.ctx = crc;
...
return *(u32 *)desc.ctx;
Does this raise any byte swapping issues? Looking at how the crc32
code in crypto/ works, I'm almost certain this is broken on big-endian
systems, and we need to add le32_to_cup() and cpu_to_le32() calls here.
Am I missing something?
- Ted
On Thu, Nov 08, 2012 at 10:53:07AM -0500, Theodore Ts'o wrote:
> Hey Darrick,
>
> I was looking at this code a bit more closely while applying the
> revised version of this patch --- and this in particular raised a red
> flag for me:
>
> *(u32 *)desc.ctx = crc;
> ...
> return *(u32 *)desc.ctx;
>
> Does this raise any byte swapping issues? Looking at how the crc32
> code in crypto/ works, I'm almost certain this is broken on big-endian
> systems, and we need to add le32_to_cup() and cpu_to_le32() calls here.
>
> Am I missing something?
crc32_[bl]e_generic() in lib/crc32.c contains the appropriate be/le-to-cpu
conversion calls to ensure that the results are always returned in cpu-endian
format. The crypto/crc32.c file is basically just a crypto-hash wrapper around
the lib/crc32.c code, and don't touch the endianness at all. The ext4 code
receives the results from the crypto code in cpu-endian format and always
converts it to (or from) little-endian format when accessing disk structures.
As far as e2fsprogs, it leaves the checksums in le format, only converting it
to cpu-endian format when verifying or setting the checksum.
--D
>
> - Ted
On Fri, Nov 09, 2012 at 03:19:12PM -0800, Darrick J. Wong wrote:
> On Thu, Nov 08, 2012 at 10:53:07AM -0500, Theodore Ts'o wrote:
> > Hey Darrick,
> >
> > I was looking at this code a bit more closely while applying the
> > revised version of this patch --- and this in particular raised a red
> > flag for me:
> >
> > *(u32 *)desc.ctx = crc;
> > ...
> > return *(u32 *)desc.ctx;
> >
> > Does this raise any byte swapping issues? Looking at how the crc32
> > code in crypto/ works, I'm almost certain this is broken on big-endian
> > systems, and we need to add le32_to_cup() and cpu_to_le32() calls here.
> >
> > Am I missing something?
>
> crc32_[bl]e_generic() in lib/crc32.c contains the appropriate be/le-to-cpu
> conversion calls to ensure that the results are always returned in cpu-endian
> format. The crypto/crc32.c file is basically just a crypto-hash wrapper around
> the lib/crc32.c code, and don't touch the endianness at all. The ext4 code
> receives the results from the crypto code in cpu-endian format and always
> converts it to (or from) little-endian format when accessing disk structures.
>
> As far as e2fsprogs, it leaves the checksums in le format, only converting it
> to cpu-endian format when verifying or setting the checksum.
I forgot to mention -- I tried creating a metadata_csum filesystem on an x86
box. Then I bounced it between that machine and my one remaining ppc32 box,
running a mount/read/write/unmount/fsck script on each bounce. I didn't see
any errors. Sadly, I no longer have anything weirder (sparc64/s390x) to test.
--D
>
> --D
> >
> > - Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html