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
