| Message ID | 20231223181101.1954-4-gregory.price@memverge.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mempolicy2, mbind2, and weighted interleave | expand |
On Wed, Dec 27, 2023 at 04:39:29PM +0800, Huang, Ying wrote: > Gregory Price <gourry.memverge@gmail.com> writes: > > > + * fields and return just the mode in mode_arg and flags in flags. > > + */ > > +static inline int sanitize_mpol_flags(int *mode_arg, unsigned short *flags) > > +{ > > + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS); > > + > > + *flags = *mode_arg & MPOL_MODE_FLAGS; > > + *mode_arg = mode; > > It appears that it's unnecessary to introduce a local variable to split > mode/flags. Just reuse the original code? > ack ~Gregory
On Tue, Dec 26, 2023 at 02:05:35AM -0500, Gregory Price wrote: > On Wed, Dec 27, 2023 at 04:39:29PM +0800, Huang, Ying wrote: > > Gregory Price <gourry.memverge@gmail.com> writes: > > > > > + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS); > > > + > > > + *flags = *mode_arg & MPOL_MODE_FLAGS; > > > + *mode_arg = mode; > > > > It appears that it's unnecessary to introduce a local variable to split > > mode/flags. Just reuse the original code? > > Revisiting during fixes: Note the change from int to short. I chose to make this explicit because validate_mpol_flags takes a short. I'm fairly sure changing it back throws a truncation warning. ~Gregroy
Gregory Price <gourry.memverge@gmail.com> writes: > split sanitize_mpol_flags into sanitize and validate. > > Sanitize is used by set_mempolicy to split (int mode) into mode > and mode_flags, and then validates them. > > Validate validates already split flags. > > Validate will be reused for new syscalls that accept already > split mode and mode_flags. > > Signed-off-by: Gregory Price <gregory.price@memverge.com> > --- > mm/mempolicy.c | 29 ++++++++++++++++++++++------- > 1 file changed, 22 insertions(+), 7 deletions(-) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index 0a180c670f0c..59ac0da24f56 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -1463,24 +1463,39 @@ static int copy_nodes_to_user(unsigned long __user *mask, unsigned long maxnode, > return copy_to_user(mask, nodes_addr(*nodes), copy) ? -EFAULT : 0; > } > > -/* Basic parameter sanity check used by both mbind() and set_mempolicy() */ > -static inline int sanitize_mpol_flags(int *mode, unsigned short *flags) > +/* > + * Basic parameter sanity check used by mbind/set_mempolicy > + * May modify flags to include internal flags (e.g. MPOL_F_MOF/F_MORON) > + */ > +static inline int validate_mpol_flags(unsigned short mode, unsigned short *flags) > { > - *flags = *mode & MPOL_MODE_FLAGS; > - *mode &= ~MPOL_MODE_FLAGS; > - > - if ((unsigned int)(*mode) >= MPOL_MAX) > + if ((unsigned int)(mode) >= MPOL_MAX) > return -EINVAL; > if ((*flags & MPOL_F_STATIC_NODES) && (*flags & MPOL_F_RELATIVE_NODES)) > return -EINVAL; > if (*flags & MPOL_F_NUMA_BALANCING) { > - if (*mode != MPOL_BIND) > + if (mode != MPOL_BIND) > return -EINVAL; > *flags |= (MPOL_F_MOF | MPOL_F_MORON); > } > return 0; > } > > +/* > + * Used by mbind/set_memplicy to split and validate mode/flags > + * set_mempolicy combines (mode | flags), split them out into separate mbind() uses mode flags too. > + * fields and return just the mode in mode_arg and flags in flags. > + */ > +static inline int sanitize_mpol_flags(int *mode_arg, unsigned short *flags) > +{ > + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS); > + > + *flags = *mode_arg & MPOL_MODE_FLAGS; > + *mode_arg = mode; It appears that it's unnecessary to introduce a local variable to split mode/flags. Just reuse the original code? > + > + return validate_mpol_flags(mode, flags); > +} > + > static long kernel_mbind(unsigned long start, unsigned long len, > unsigned long mode, const unsigned long __user *nmask, > unsigned long maxnode, unsigned int flags) -- Best Regards, Huang, Ying
Gregory Price <gregory.price@memverge.com> writes: > On Tue, Dec 26, 2023 at 02:05:35AM -0500, Gregory Price wrote: >> On Wed, Dec 27, 2023 at 04:39:29PM +0800, Huang, Ying wrote: >> > Gregory Price <gourry.memverge@gmail.com> writes: >> > >> > > + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS); >> > > + >> > > + *flags = *mode_arg & MPOL_MODE_FLAGS; >> > > + *mode_arg = mode; >> > >> > It appears that it's unnecessary to introduce a local variable to split >> > mode/flags. Just reuse the original code? >> > > > Revisiting during fixes: Note the change from int to short. > > I chose to make this explicit because validate_mpol_flags takes a short. > > I'm fairly sure changing it back throws a truncation warning. Why something like below doesn't work? int sanitize_mpol_flags(int *mode, unsigned short *flags) { *flags = *mode & MPOL_MODE_FLAGS; *mode &= ~MPOL_MODE_FLAGS; return validate_mpol_flags(*mode, flags); } -- Best Regards, Huang, Ying
On Tue, Jan 02, 2024 at 05:09:55PM +0800, Huang, Ying wrote: > Gregory Price <gregory.price@memverge.com> writes: > > > On Tue, Dec 26, 2023 at 02:05:35AM -0500, Gregory Price wrote: > >> On Wed, Dec 27, 2023 at 04:39:29PM +0800, Huang, Ying wrote: > >> > Gregory Price <gourry.memverge@gmail.com> writes: > >> > > >> > > + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS); > >> > > + > >> > > + *flags = *mode_arg & MPOL_MODE_FLAGS; > >> > > + *mode_arg = mode; > >> > > >> > It appears that it's unnecessary to introduce a local variable to split > >> > mode/flags. Just reuse the original code? > >> > > > > > Revisiting during fixes: Note the change from int to short. > > > > I chose to make this explicit because validate_mpol_flags takes a short. > > > > I'm fairly sure changing it back throws a truncation warning. > > Why something like below doesn't work? > > int sanitize_mpol_flags(int *mode, unsigned short *flags) > { > *flags = *mode & MPOL_MODE_FLAGS; > *mode &= ~MPOL_MODE_FLAGS; > > return validate_mpol_flags(*mode, flags); > } was concerned with silent truncation of (*mode) (int) to short. *shrug* happy to change it ~Gregory
diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 0a180c670f0c..59ac0da24f56 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -1463,24 +1463,39 @@ static int copy_nodes_to_user(unsigned long __user *mask, unsigned long maxnode, return copy_to_user(mask, nodes_addr(*nodes), copy) ? -EFAULT : 0; } -/* Basic parameter sanity check used by both mbind() and set_mempolicy() */ -static inline int sanitize_mpol_flags(int *mode, unsigned short *flags) +/* + * Basic parameter sanity check used by mbind/set_mempolicy + * May modify flags to include internal flags (e.g. MPOL_F_MOF/F_MORON) + */ +static inline int validate_mpol_flags(unsigned short mode, unsigned short *flags) { - *flags = *mode & MPOL_MODE_FLAGS; - *mode &= ~MPOL_MODE_FLAGS; - - if ((unsigned int)(*mode) >= MPOL_MAX) + if ((unsigned int)(mode) >= MPOL_MAX) return -EINVAL; if ((*flags & MPOL_F_STATIC_NODES) && (*flags & MPOL_F_RELATIVE_NODES)) return -EINVAL; if (*flags & MPOL_F_NUMA_BALANCING) { - if (*mode != MPOL_BIND) + if (mode != MPOL_BIND) return -EINVAL; *flags |= (MPOL_F_MOF | MPOL_F_MORON); } return 0; } +/* + * Used by mbind/set_memplicy to split and validate mode/flags + * set_mempolicy combines (mode | flags), split them out into separate + * fields and return just the mode in mode_arg and flags in flags. + */ +static inline int sanitize_mpol_flags(int *mode_arg, unsigned short *flags) +{ + unsigned short mode = (*mode_arg & ~MPOL_MODE_FLAGS); + + *flags = *mode_arg & MPOL_MODE_FLAGS; + *mode_arg = mode; + + return validate_mpol_flags(mode, flags); +} + static long kernel_mbind(unsigned long start, unsigned long len, unsigned long mode, const unsigned long __user *nmask, unsigned long maxnode, unsigned int flags)
split sanitize_mpol_flags into sanitize and validate. Sanitize is used by set_mempolicy to split (int mode) into mode and mode_flags, and then validates them. Validate validates already split flags. Validate will be reused for new syscalls that accept already split mode and mode_flags. Signed-off-by: Gregory Price <gregory.price@memverge.com> --- mm/mempolicy.c | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-)