lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 11 Dec 2022 22:39:44 +0000 From: Al Viro <viro@...iv.linux.org.uk> To: "Fabio M. De Francesco" <fmdefrancesco@...il.com> Cc: Evgeniy Dushistov <dushistov@...l.ru>, Ira Weiny <ira.weiny@...el.com>, linux-kernel@...r.kernel.org, bpf@...r.kernel.org, linux-fsdevel@...r.kernel.org Subject: Re: [PATCH 3/3] fs/ufs: Replace kmap() with kmap_local_page() 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?)
Powered by blists - more mailing lists