Re: [PATCH v1] mm: bdi: reset min_ratio after the sysfs attributes are gone

[Jan Kara]

On Fri 16-10-26 23:06:20, Nickolai Zeldovich wrote:
> A write to /sys/class/bdi/<dev>/min_ratio (or min_ratio_fine or
> min_bytes) that races with the removal of the device leaves the global
> sum of minimum dirty shares charged for a bdi that no longer exists.
> Until the next reboot, that share of the dirty threshold is withheld
> from every other bdi (__wb_calc_thresh() scales by 100 % minus the
> sum), and a later min_ratio store on any bdi that would push the sum
> to 100 % fails with EINVAL although every bdi reads 0. Root privilege
> is needed for the store; the removal is any del_gendisk() (a disk or
> loop device going away) or the unmount of a filesystem with its own
> bdi. On an x86_64 defconfig kernel (v7.3-rc7), 2000 rounds of
> LOOP_CTL_ADD/LOOP_CTL_REMOVE on one loop device with a second thread
> writing 50 to its min_ratio leak the share on the first round the
> store lands in the window; afterwards `echo 99 > min_ratio` on another
> loop device fails with EINVAL. With the fix the same run keeps the sum
> at 0 and the store succeeds. Affected since v5.16, when the reset in
> bdi_unregister() was added.
> 
> bdi_unregister() gives the bdi's share back with bdi_set_min_ratio(bdi,
> 0) and only then calls device_unregister(), which removes the sysfs
> attributes. A min_ratio_store() that gets its kernfs active reference
> before the removal runs to completion after the reset:
> __bdi_set_min_ratio() takes bdi_lock, sees bdi->min_ratio == 0, adds
> the new value to bdi_min_ratio and stores it in the bdi, which
> bdi_put() then frees. Nothing gives that share back.
> 
> Move the reset after device_unregister(): once the attributes are gone
> no store can run, and bdi_set_min_ratio() needs nothing from the
> device.
> 
> Found while formally verifying bdi_unregister() against the kernel
> binary.
> 
> Fixes: 3c376dfafbf7 ("mm: bdi: initialize bdi_min_ratio when bdi is unregistered")
> Assisted-by: LLM
> Signed-off-by: Nickolai Zeldovich <nickolai@xxxxxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> 
> Notes:
>     The window is between the reset and kernfs draining the attribute's active
>     references in device_del(): bdi_debug_unregister() and the device
>     removal (uevent, bus and class teardown) sit in it, tens of microseconds,
>     so a tight store loop lands in it within a few hundred add/remove rounds
>     under QEMU TCG. A store that lands there also fails every later min_ratio
>     store of 50 with EINVAL, which is why the unpatched run shows few landed
>     stores: the leaked 50 % plus a new 50 % reach the limit. The same window
>     exists for min_ratio_fine and min_bytes (same helper), and for the
>     per-superblock bdis unregistered from generic_shutdown_super().
>     
>     Reproducer (x86_64 defconfig, root, loop devices 20 and 21 free, QEMU
>     -smp 2):
>     
>       gcc -O2 -pthread -static -o bdi_min_ratio_leak_short bdi_min_ratio_leak_short.c
>       ./bdi_min_ratio_leak_short
>       before: min_ratio 99 on loop21: Invalid argument   (exit 1)
>       after:  min_ratio 99 on loop21: ok                 (exit 0)
>     
>       /* loop device add/remove racing a min_ratio store; root, CONFIG_BLK_DEV_LOOP */
>       #include <errno.h>
>       #include <fcntl.h>
>       #include <linux/loop.h>
>       #include <pthread.h>
>       #include <stdio.h>
>       #include <string.h>
>       #include <sys/ioctl.h>
>       #include <unistd.h>
>     
>       static volatile int stop;
>     
>       static int store(const char *path, const char *val)
>       {
>       	int fd = open(path, O_WRONLY), r;
>     
>       	if (fd < 0)
>       		return -1;
>       	r = write(fd, val, strlen(val));
>       	close(fd);
>       	return r < 0 ? -errno : 0;
>       }
>     
>       static void *storer(void *arg)
>       {
>       	while (!stop)
>       		store("/sys/class/bdi/7:20/min_ratio", "50");
>       	return NULL;
>       }
>     
>       int main(void)
>       {
>       	int ctl = open("/dev/loop-control", O_RDWR), i, r;
>       	pthread_t th;
>     
>       	ioctl(ctl, LOOP_CTL_ADD, 21);			/* the control bdi */
>       	pthread_create(&th, NULL, storer, NULL);
>       	for (i = 0; i < 2000; i++) {
>       		if (ioctl(ctl, LOOP_CTL_ADD, 20) < 0)
>       			continue;
>       		while (ioctl(ctl, LOOP_CTL_REMOVE, 20) < 0)
>       			;
>       	}
>       	stop = 1;
>       	pthread_join(th, NULL);
>       	r = store("/sys/class/bdi/7:21/min_ratio", "99");	/* sum must stay < 100 */
>       	printf("min_ratio 99 on loop21: %s\n", r ? strerror(-r) : "ok");
>       	store("/sys/class/bdi/7:21/min_ratio", "0");
>       	return r != 0;
>       }
> 
>  mm/backing-dev.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 18e999053bae..fad1495d3b20 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -1162,19 +1162,20 @@ void bdi_unregister(struct backing_dev_info *bdi)
>  	wb_shutdown(&bdi->wb);
>  	cgwb_bdi_unregister(bdi);
>  
> -	/*
> -	 * If this BDI's min ratio has been set, use bdi_set_min_ratio() to
> -	 * update the global bdi_min_ratio.
> -	 */
> -	if (bdi->min_ratio)
> -		bdi_set_min_ratio(bdi, 0);
> -
>  	if (bdi->dev) {
>  		bdi_debug_unregister(bdi);
>  		device_unregister(bdi->dev);
>  		bdi->dev = NULL;
>  	}
>  
> +	/*
> +	 * If this BDI's min ratio has been set, use bdi_set_min_ratio() to
> +	 * update the global bdi_min_ratio. After device_unregister(): the
> +	 * sysfs min_ratio stores can no longer run and set it again.
> +	 */
> +	if (bdi->min_ratio)
> +		bdi_set_min_ratio(bdi, 0);
> +
>  	if (bdi->owner) {
>  		put_device(bdi->owner);
>  		bdi->owner = NULL;
> 
> base-commit: 70dd4ee72eb2d5793c62e71c14e3aba78e5a4e56
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR