Re: [RFC][PATCH 5/7] x86, mpx: shrink per-mm MPX data

[Thomas Gleixner]

On Wed, 1 Feb 2017, Dave Hansen wrote:
>  /*
> - * NULL is theoretically a valid place to put the bounds
> - * directory, so point this at an invalid address.
> + * These get stored into mm_context_t->mpx_directory_info.
> + * We could theoretically use bits 0 and 1, but those are
> + * used in the BNDCFGU register that also holds the bounds
> + * directory pointer.  To avoid confusion, use different bits.
>   */
> -#define MPX_INVALID_BOUNDS_DIR	((void __user *)-1)
> +#define MPX_INVALID_BOUNDS_DIR	(1UL<<2)
> +#define MPX_LARGE_BOUNDS_DIR	(1UL<<3)

Please keep them tabular aligned

>  static inline int mpx_bd_size_shift(struct mm_struct *mm)
>  {
> -	return mm->context.mpx_bd_shift;
> +	if (!kernel_managing_mpx_tables(mm))
> +		return 0;
> +	if (mm->context.mpx_directory_info & MPX_LARGE_BOUNDS_DIR)
> +		return MPX_LARGE_BOUNDS_DIR_SHIFT;
> +	return 0;

So now makes the inline sense.

> -int mpx_set_mm_bd_size(unsigned long bd_size)
> +int mpx_set_dir_size(unsigned long bd_size, unsigned long *mpx_directory_info)
>  {
>  	struct mm_struct *mm = current->mm;
> +	int ret = 0;
> +	bool large_dir = false;

>  	struct mm_struct *mm = current->mm;
> +	bool large_dir = false;
> +	int ret = 0;

Please

>  
>  	switch ((unsigned long long)bd_size) {
>  	case 0:
> -		/* Legacy call to prctl(): */
> -		mm->context.mpx_bd_shift = 0;
> -		return 0;
> +		/* Legacy call to prctl() */
> +		break;
>  	case MPX_BD_SIZE_BYTES_32:
>  		/* 32-bit, legacy-sized bounds directory: */
> -		if (is_64bit_mm(mm))
> -			return -EINVAL;
> -		mm->context.mpx_bd_shift = 0;
> -		return 0;
> +		if (is_64bit_mm(mm)) {
> +			ret = -EINVAL;
> +			break;

Why do you want to break in the error case instead of just returning the
error? In case of error it really makes no sense to fiddle with the large
page bit in the directory_info.

> +		}
> +		ret = 0;

It's already 0

> +		break;
>  	case MPX_BD_BASE_SIZE_BYTES_64:
>  		/* 64-bit, legacy-sized bounds directory: */
>  		if (!is_64bit_mm(mm)
>  		// FIXME && ! opted-in to larger address space
>  		)
> -			return -EINVAL;
> -		mm->context.mpx_bd_shift = 0;
> -		return 0;
> +			ret = -EINVAL;

See above

> +		break;
>  	case MPX_BD_BASE_SIZE_BYTES_64 << MPX_LARGE_BOUNDS_DIR_SHIFT:
>  		/*
>  		 * Non-legacy call, with larger directory.
> @@ -370,7 +372,7 @@ int mpx_set_mm_bd_size(unsigned long bd_
>  		 * change sizes.
>  		 */
>  		if (!is_64bit_mm(mm))
> -			return -EINVAL;
> +			ret = -EINVAL;

Ditto

>  		/*
>  		 * Do not let this be enabled unles we are on
>  		 * 5-level hardware *and* have that feature
> @@ -379,16 +381,20 @@ int mpx_set_mm_bd_size(unsigned long bd_
>  		if (!cpu_feature_enabled(X86_FEATURE_LA57)
>  		// FIXME && opted into larger address space
>  		)
> -			return -EINVAL;
> -		mm->context.mpx_bd_shift = MPX_LARGE_BOUNDS_DIR_SHIFT;
> -		return 0;
> +			ret = -EINVAL;
> +		if (ret)
> +			break;

This is outright silly.

> +		large_dir = true;
> +		break;
>  	}
> -	return -EINVAL;
> +	if (large_dir)
> +		(*mpx_directory_info) |= MPX_LARGE_BOUNDS_DIR;
> +	return ret;
>  }
>  
>  int mpx_enable_management(unsigned long bd_size)
>  {
> -	void __user *bd_base = MPX_INVALID_BOUNDS_DIR;
> +	void __user *bd_base;
>  	struct mm_struct *mm = current->mm;
>  	int ret = 0;
>  
> @@ -404,13 +410,16 @@ int mpx_enable_management(unsigned long
>  	 * unmap path; we can just use mm->context.bd_addr instead.
>  	 */
>  	bd_base = mpx_get_bounds_dir();
> +	if (bd_base == MPX_INVALID_BOUNDS_DIR)
> +		return -ENXIO;
> +
>  	down_write(&mm->mmap_sem);
> -	ret = mpx_set_mm_bd_size(bd_size);
> +	/* Mask out the invalid bit: */
> +	mm->context.mpx_directory_info &= ~MPX_INVALID_BOUNDS_DIR;

The handling of that bit is really confusing

> +	ret = mpx_set_dir_size(bd_size, &mm->context.mpx_directory_info);
>  	if (ret)
>  		goto out;

And what makes the thing invalid again in case of ret != 0?

> -	mm->context.bd_addr = bd_base;
> -	if (mm->context.bd_addr == MPX_INVALID_BOUNDS_DIR)
> -		ret = -ENXIO;
> +	mm->context.mpx_directory_info |= bd_base;
>  out:
>  	up_write(&mm->mmap_sem);
>  	return ret;

Thanks,

	tglx