[PATCH -next v2] fs-verity: Use struct_size() helper in enable_verity()

[PATCH -next v2] fs-verity: Use struct_size() helper in enable_verity()

[Zhang Jianhua]

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(-)

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;
 
-- 
2.31.0

Re: [PATCH -next v2] fs-verity: Use struct_size() helper in enable_verity()

[Eric Biggers]

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

Re: [PATCH -next v2] fs-verity: Use struct_size() helper in enable_verity()

[Eric Biggers]

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

Re: [PATCH -next v2] fs-verity: Use struct_size() helper in enable_verity()

[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

Re: [PATCH -next v2] fs-verity: Use struct_size() helper in enable_verity()

[zhangjianhua (E)]

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
> .

Re: [PATCH -next v2] fs-verity: Use struct_size() helper in enable_verity()

[Johan Hovold]

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

Re: [PATCH -next v2] fs-verity: Use struct_size() helper in enable_verity()

[Eric Biggers]

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