Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 11 Dec 2022 22:39:44 +0000
From:   Al Viro <viro@zeniv.linux.org.uk>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc:     Evgeniy Dushistov <dushistov@mail.ru>,
        Ira Weiny <ira.weiny@intel.com>, linux-kernel@vger.kernel.org,
        bpf@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 3/3] fs/ufs: Replace kmap() with kmap_local_page()
Message-ID: <Y5ZcMPzPG9h6C9eh@ZenIV>
References: <20221211213111.30085-1-fmdefrancesco@gmail.com>
 <20221211213111.30085-4-fmdefrancesco@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221211213111.30085-4-fmdefrancesco@gmail.com>
Sender: Al Viro <viro@ftp.linux.org.uk>
Precedence: bulk

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?)