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;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Al Viro <viro@zeniv.linux.org.uk>
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()
Date:   Mon, 12 Dec 2022 01:14:44 +0100
Message-ID: <4792154.31r3eYUQgx@suse>
In-Reply-To: <Y5ZcMPzPG9h6C9eh@ZenIV>
References: <20221211213111.30085-1-fmdefrancesco@gmail.com>
 <20221211213111.30085-4-fmdefrancesco@gmail.com> <Y5ZcMPzPG9h6C9eh@ZenIV>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On domenica 11 dicembre 2022 23:39:44 CET Al Viro wrote:
> 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.
>
I don't know why gcc didn't catch this (gcc version 12.2.1 20221020 [revision 
0aaef83351473e8f4eb774f8f999bbe87a4866d7] (SUSE Linux)):

setarch i686
make ARCH=i386 O=../build-linux-x86_32-debug/ -j12
make[1]: Entering directory '/usr/src/git/kernels/build-linux-x86_32-debug'
  GEN     Makefile
  DESCEND bpf/resolve_btfids
  CALL    /usr/src/git/kernels/linux/scripts/checksyscalls.sh
  CC [M]  fs/ufs/dir.o
  LD [M]  fs/ufs/ufs.o
  MODPOST Module.symvers
Kernel: arch/x86/boot/bzImage is ready  (#3)
  LD [M]  fs/ufs/ufs.ko
  BTF [M] fs/ufs/ufs.ko
make[1]: Leaving directory '/usr/src/git/kernels/build-linux-x86_32-debug'

> Here kaddr is obviously uninitialized and compiler would've
> probably caught that.
>
I'd better use option W=1 next time.
>
> 	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.
>
I'm sorry that I entirely missed this :-(
> 
> > @@ -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?)
>
I don't yet know. Maybe that it is related to a copy-paste error or something 
like that...

As said, I'll send next version ASAP.

Again thanks for your kind help,

Fabio