Re: [PATCH 3/3] fs/ufs: Replace kmap() with kmap_local_page()

[Al Viro]

On Sun, Dec 11, 2022 at 10:31:11PM +0100, Fabio M. De Francesco wrote:

> +/*
> + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the
> + * rules documented in kmap_local_page()/kunmap_local().
> + *
> + * NOTE: ufs_find_entry() and ufs_dotdot() act as calls to ufs_get_page()
> + * and must be treated accordingly for nesting purposes.
> + */
>  static void *ufs_get_page(struct inode *dir, unsigned long n, struct page **page)
>  {
> +	char *kaddr;
> +
>  	struct address_space *mapping = dir->i_mapping;
>  	*page = read_mapping_page(mapping, n, NULL);
>  	if (!IS_ERR(*page)) {
> -		kmap(*page);
> +		kmap_local_page(*page);
>  		if (unlikely(!PageChecked(*page))) {
> -			if (!ufs_check_page(*page))
> +			if (!ufs_check_page(*page, kaddr))

	Er...  Building the patched tree is occasionally useful.
Here kaddr is obviously uninitialized and compiler would've
probably caught that.

	And return value of kmap_local_page() is lost, which
is related to the previous issue ;-)


>  				goto fail;
>  		}
>  	}
> -	return page;
> +	return *page;

Hell, no.  Callers expect the pointer to the first byte of
your page.  What it should return is kaddr.

> @@ -388,7 +406,8 @@ int ufs_add_link(struct dentry *dentry, struct inode *inode)
>  	mark_inode_dirty(dir);
>  	/* OFFSET_CACHE */
>  out_put:
> -	ufs_put_page(page);
> +	ufs_put_page(page, kaddr);
> +	return 0;
>  out_unlock:
>  	unlock_page(page);
>  	goto out_put;

That can't be right.  Places like
        if (err)
		goto out_unlock;
do not expect err to be lost.  You end up returning 0 now.  Something strange
happened here (in the previous commit, perhaps?)