On Thu 09-10-25 09:59:28, Mateusz Guzik wrote:
> ... to make sure all accesses are properly validated.
>
> Merely renaming the var to __i_state still lets the compiler make the
> following suggestion:
> error: 'struct inode' has no member named 'i_state'; did you mean '__i_state'?
>
> Unfortunately some people will add the __'s and call it a day.
>
> In order to make it harder to mess up in this way, hide it behind a
> struct. The resulting error message should be convincing in terms of
> checking what to do:
> error: invalid operands to binary & (have 'struct inode_state_flags' and 'int')
>
> Of course people determined to do a plain access can still do it, but
> nothing can be done for that case.
>
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> include/linux/fs.h | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 77b6486dcae7..21c73df3ce75 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -785,6 +785,13 @@ enum inode_state_flags_enum {
> #define I_DIRTY (I_DIRTY_INODE | I_DIRTY_PAGES)
> #define I_DIRTY_ALL (I_DIRTY | I_DIRTY_TIME)
>
> +/*
> + * Use inode_state_read() & friends to access.
> + */
> +struct inode_state_flags {
> + enum inode_state_flags_enum __state;
> +};
> +
> /*
> * Keep mostly read-only and often accessed (especially for
> * the RCU path lookup and 'stat' data) fields at the beginning
> @@ -843,7 +850,7 @@ struct inode {
> #endif
>
> /* Misc */
> - enum inode_state_flags_enum i_state;
> + struct inode_state_flags i_state;
> /* 32-bit hole */
> struct rw_semaphore i_rwsem;
>
> @@ -909,19 +916,19 @@ struct inode {
> */
> static inline enum inode_state_flags_enum inode_state_read_once(struct inode *inode)
> {
> - return READ_ONCE(inode->i_state);
> + return READ_ONCE(inode->i_state.__state);
> }
>
> static inline enum inode_state_flags_enum inode_state_read(struct inode *inode)
> {
> lockdep_assert_held(&inode->i_lock);
> - return inode->i_state;
> + return inode->i_state.__state;
> }
>
> static inline void inode_state_set_raw(struct inode *inode,
> enum inode_state_flags_enum flags)
> {
> - WRITE_ONCE(inode->i_state, inode->i_state | flags);
> + WRITE_ONCE(inode->i_state.__state, inode->i_state.__state | flags);
> }
>
> static inline void inode_state_set(struct inode *inode,
> @@ -934,7 +941,7 @@ static inline void inode_state_set(struct inode *inode,
> static inline void inode_state_clear_raw(struct inode *inode,
> enum inode_state_flags_enum flags)
> {
> - WRITE_ONCE(inode->i_state, inode->i_state & ~flags);
> + WRITE_ONCE(inode->i_state.__state, inode->i_state.__state & ~flags);
> }
>
> static inline void inode_state_clear(struct inode *inode,
> @@ -947,7 +954,7 @@ static inline void inode_state_clear(struct inode *inode,
> static inline void inode_state_assign_raw(struct inode *inode,
> enum inode_state_flags_enum flags)
> {
> - WRITE_ONCE(inode->i_state, flags);
> + WRITE_ONCE(inode->i_state.__state, flags);
> }
>
> static inline void inode_state_assign(struct inode *inode,
> @@ -962,7 +969,7 @@ static inline void inode_state_replace_raw(struct inode *inode,
> enum inode_state_flags_enum setflags)
> {
> enum inode_state_flags_enum flags;
> - flags = inode->i_state;
> + flags = inode->i_state.__state;
> flags &= ~clearflags;
> flags |= setflags;
> inode_state_assign_raw(inode, flags);
> --
> 2.34.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR