| Message ID | 1351622404-18214-2-git-send-email-behanw@converseincode.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Oct 30, 2012 at 02:40:04PM -0400, Behan Webster wrote: > From: Mark Charlebois <charlebm@gmail.com> > > 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 <charlebm@gmail.com> > Signed-off-by: Behan Webster <behanw@converseincode.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <charlebm@gmail.com> >> >> 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 <charlebm@gmail.com> >> Signed-off-by: Behan Webster <behanw@converseincode.com> > 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
On Tue, Oct 30, 2012 at 02:40:04PM -0400, Behan Webster wrote: > From: Mark Charlebois <charlebm@gmail.com> > > 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 <charlebm@gmail.com> > Signed-off-by: Behan Webster <behanw@converseincode.com> > --- > 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 <darrick.wong@oracle.com> --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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org 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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;