Re: [PATCH] btrfs: Introduce compile time structure size check

[David Sterba <dsterba@suse.cz>]

On Fri, Jul 13, 2018 at 10:36:42PM +0800, Qu Wenruo wrote:
> 
> 
> On 2018年07月13日 22:34, David Sterba wrote:
> > On Thu, Jul 12, 2018 at 02:19:07PM +0800, Qu Wenruo wrote:
> >> Introduce a new macro based compile time check for ioctl structures.
> >>
> >> The new macro is BTRFS_ASSERT_SIZE(), which is mostly copied from
> >> VMMDEV_ASSERT_SIZE().
> > 
> > The macro should be generic, there's nothing specific to btrfs. There's
> > a similar one in the progs.
> > 
> >> Such check is only added to structure pended to power of 2.
> > 
> > There's no constraint about power of 2 sizes, it works for any size.
> 
> While some structure doesn't do padding at all, and it looks like they
> may get expanded later.
> Do we really need to limit the size right now?

The size of an ioctl structure is part of kernel<->userspace API and
must not change once public. The ioctl number is calculated using the
structure size.

> >> And exposed one structure, btrfs_ioctl_get_dev_stats() is not aligned
> >> well.
> >> The misalign is introduced by commit b27f7c0c150f ("btrfs: join DEV_STATS
> >> ioctls to one").
> > 
> > Yeah, that was an oversight, so the correct value to check against is
> > 1024 + 8 = 1032, see ioctl.h in progs.
> 
> Can't we just revert to 1024?

Answered by the above, we absolutely cannot change the numbers now.

> That plus 8 doesn't really look well, and shrink the size shouldn't
> bring any problem AFAIK.

See

https://elixir.bootlin.com/linux/latest/source/include/uapi/asm-generic/ioctl.h#L88

for _IOWR definition,

btrfs-progs:ioctl.h

817 #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
818                                       struct btrfs_ioctl_get_dev_stats)

kernel:fs/btrfs/ioctl.c

5914         case BTRFS_IOC_GET_DEV_STATS:
5915                 return btrfs_ioctl_get_dev_stats(fs_info, argp);

The values must match, otherwise dev stats will randomly break on
old/new kernel and progs combinations.