| Message ID | 20220519022450.2434483-1-chris.zjh@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [-next,v2] fs-verity: Use struct_size() helper in enable_verity() | expand |
On Thu, May 19, 2022 at 10:24:50AM +0800, Zhang Jianhua wrote: > Also, address the following sparse warning: > fs/verity/enable.c:205:28: warning: using sizeof on a flexible structure How can I reproduce this warning? I am using the latest version of sparse, and I don't see any of these warnings you're reporting. $ sparse --version v0.6.4 $ make C=2 fs/verity/ CHECK scripts/mod/empty.c CALL scripts/checksyscalls.sh CALL scripts/atomic/check-atomics.sh DESCEND objtool CHECK fs/verity/enable.c CHECK fs/verity/hash_algs.c CHECK fs/verity/init.c CHECK fs/verity/measure.c CHECK fs/verity/open.c CHECK fs/verity/read_metadata.c CHECK fs/verity/verify.c CHECK fs/verity/signature.c - Eric
On Wed, May 18, 2022 at 08:06:04PM -0700, Eric Biggers wrote: > On Thu, May 19, 2022 at 10:24:50AM +0800, Zhang Jianhua wrote: > > Also, address the following sparse warning: > > fs/verity/enable.c:205:28: warning: using sizeof on a flexible structure > > How can I reproduce this warning? I am using the latest version of sparse, and > I don't see any of these warnings you're reporting. > > $ sparse --version > v0.6.4 > $ make C=2 fs/verity/ > CHECK scripts/mod/empty.c > CALL scripts/checksyscalls.sh > CALL scripts/atomic/check-atomics.sh > DESCEND objtool > CHECK fs/verity/enable.c > CHECK fs/verity/hash_algs.c > CHECK fs/verity/init.c > CHECK fs/verity/measure.c > CHECK fs/verity/open.c > CHECK fs/verity/read_metadata.c > CHECK fs/verity/verify.c > CHECK fs/verity/signature.c > 'make C=2 CHECK="sparse -Wflexible-array-sizeof"' does the trick. However, it produces a *lot* of warnings all over the place. Unless there is an effort to actually address all of these so that this warning can be enabled by default, I don't see the poinnt in addressing these just for the warnings sake. The change to fsverity_ioctl_measure() is definitely just for the warning's sake, so I don't really want to do that one. The change to enable_verity() is a bit less useless, so I could still take that one. - Eric
[Please use reply all, not just reply!] On Thu, May 19, 2022 at 11:54:48AM +0800, zhangjianhua (E) wrote: > Hi Eric > > The warnings in commit message are from the build log in Jan 2022, and I > find these sizeof are still here, so I submit > > these two patches. I build the kernel just now and encounter the same > situation with you, there are lots of warnings. > > Maybe you are right, there should be some mechanism to solve this problem > completely. > > I've updated the commit message and applied this patch, but not the other one, as the other one wasn't actually dealing with a variable length which made it pretty much pointless, as I mentioned. If you'd like to look into making sparse enable this warning by default, I'd certainly encourage you to do so. But it looks like the warning itself could use some more work. It probably should only warn if the sizeof(struct_with_flexible_array) is actually being added to another value, and where that value is not a compile-time constant. - Eric
Thanks, I will do more work about sparse and maybe find some answers. Zhang Jianhua 在 2022/5/19 12:22, Eric Biggers 写道: > [Please use reply all, not just reply!] > > On Thu, May 19, 2022 at 11:54:48AM +0800, zhangjianhua (E) wrote: >> Hi Eric >> >> The warnings in commit message are from the build log in Jan 2022, and I >> find these sizeof are still here, so I submit >> >> these two patches. I build the kernel just now and encounter the same >> situation with you, there are lots of warnings. >> >> Maybe you are right, there should be some mechanism to solve this problem >> completely. >> >> > I've updated the commit message and applied this patch, but not the other one, > as the other one wasn't actually dealing with a variable length which made it > pretty much pointless, as I mentioned. > > If you'd like to look into making sparse enable this warning by default, I'd > certainly encourage you to do so. But it looks like the warning itself could > use some more work. It probably should only warn if the > sizeof(struct_with_flexible_array) is actually being added to another value, and > where that value is not a compile-time constant. > > - Eric > .
On Wed, May 18, 2022 at 08:17:59PM -0700, Eric Biggers wrote: > On Wed, May 18, 2022 at 08:06:04PM -0700, Eric Biggers wrote: > > On Thu, May 19, 2022 at 10:24:50AM +0800, Zhang Jianhua wrote: > > > Also, address the following sparse warning: > > > fs/verity/enable.c:205:28: warning: using sizeof on a flexible structure > > > > How can I reproduce this warning? I am using the latest version of sparse, and > > I don't see any of these warnings you're reporting. > > > > $ sparse --version > > v0.6.4 > > $ make C=2 fs/verity/ > > CHECK scripts/mod/empty.c > > CALL scripts/checksyscalls.sh > > CALL scripts/atomic/check-atomics.sh > > DESCEND objtool > > CHECK fs/verity/enable.c > > CHECK fs/verity/hash_algs.c > > CHECK fs/verity/init.c > > CHECK fs/verity/measure.c > > CHECK fs/verity/open.c > > CHECK fs/verity/read_metadata.c > > CHECK fs/verity/verify.c > > CHECK fs/verity/signature.c > > > > 'make C=2 CHECK="sparse -Wflexible-array-sizeof"' does the trick. However, it > produces a *lot* of warnings all over the place. > > Unless there is an effort to actually address all of these so that this warning > can be enabled by default, I don't see the poinnt in addressing these just for > the warnings sake. The change to fsverity_ioctl_measure() is definitely just > for the warning's sake, so I don't really want to do that one. The change to > enable_verity() is a bit less useless, so I could still take that one. Importantly, struct_size() still relies on sizeof() so this has zero effect on those sparse warnings. Johan
On Thu, May 19, 2022 at 01:24:45PM +0200, Johan Hovold wrote: > On Wed, May 18, 2022 at 08:17:59PM -0700, Eric Biggers wrote: > > On Wed, May 18, 2022 at 08:06:04PM -0700, Eric Biggers wrote: > > > On Thu, May 19, 2022 at 10:24:50AM +0800, Zhang Jianhua wrote: > > > > Also, address the following sparse warning: > > > > fs/verity/enable.c:205:28: warning: using sizeof on a flexible structure > > > > > > How can I reproduce this warning? I am using the latest version of sparse, and > > > I don't see any of these warnings you're reporting. > > > > > > $ sparse --version > > > v0.6.4 > > > $ make C=2 fs/verity/ > > > CHECK scripts/mod/empty.c > > > CALL scripts/checksyscalls.sh > > > CALL scripts/atomic/check-atomics.sh > > > DESCEND objtool > > > CHECK fs/verity/enable.c > > > CHECK fs/verity/hash_algs.c > > > CHECK fs/verity/init.c > > > CHECK fs/verity/measure.c > > > CHECK fs/verity/open.c > > > CHECK fs/verity/read_metadata.c > > > CHECK fs/verity/verify.c > > > CHECK fs/verity/signature.c > > > > > > > 'make C=2 CHECK="sparse -Wflexible-array-sizeof"' does the trick. However, it > > produces a *lot* of warnings all over the place. > > > > Unless there is an effort to actually address all of these so that this warning > > can be enabled by default, I don't see the poinnt in addressing these just for > > the warnings sake. The change to fsverity_ioctl_measure() is definitely just > > for the warning's sake, so I don't really want to do that one. The change to > > enable_verity() is a bit less useless, so I could still take that one. > > Importantly, struct_size() still relies on sizeof() so this has zero > effect on those sparse warnings. > Yeah, you're right. In fact struct_size() generates two warnings, whereas directly writing sizeof only generates 1! So clearly sparse's -Wflexible-array-sizeof warning is useless as-is. I'm still keeping this patch, but I updated the commit message to not claim that it addresses a sparse warning. Now it's just: fs-verity: Use struct_size() helper in enable_verity() Follow the best practice for allocating a variable-sized structure. - Eric
diff --git a/fs/verity/enable.c b/fs/verity/enable.c index f75d2c010f36..075dc0aa5416 100644 --- a/fs/verity/enable.c +++ b/fs/verity/enable.c @@ -201,7 +201,7 @@ static int enable_verity(struct file *filp, const struct fsverity_operations *vops = inode->i_sb->s_vop; struct merkle_tree_params params = { }; struct fsverity_descriptor *desc; - size_t desc_size = sizeof(*desc) + arg->sig_size; + size_t desc_size = struct_size(desc, signature, arg->sig_size); struct fsverity_info *vi; int err;
Make use of the struct_size() helper to calculate the size of struct fsverity_digest instead of an open-coded version, in order to get rid of the warning by sparse. Also, address the following sparse warning: fs/verity/enable.c:205:28: warning: using sizeof on a flexible structure Signed-off-by: Zhang Jianhua <chris.zjh@huawei.com> --- v2: - change the commit message from bugfix to cleanup fs/verity/enable.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)