The bug is here: if (!server || server->pnfs_curr_ld->id != dev->cbd_layout_type) {
The list iterator value 'server' will *always* be set and non-NULL by list_for_each_entry_rcu, so it is incorrect to assume that the iterator value will be NULL if the list is empty or no element is found (In fact, it will be a bogus pointer to an invalid struct object containing the HEAD, which is used for above check at next outer loop). Otherwise it may bypass the check in theory (if server->pnfs_curr_ld->id == dev->cbd_layout_type, 'server' now is a bogus pointer) and lead to invalid memory access passing the check.
Furthermore, even if we have a valid pointer, nothing pins the super block, and so the struct nfs_server could end up getting freed while we're using it.
Since all we want is a pointer to the struct pnfs_layoutdriver_type, let's skip all the iteration over super blocks, and just use API to find the layout driver directly. And to avoid use last found 'ld' which may not exists any more, just call the API for every 'dev'.
At the same time, move the code to make the logic clearer.
Cc: stable@vger.kernel.org Fixes: 1be5683b03a76 ("pnfs: CB_NOTIFY_DEVICEID") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com ---
changes since v1: - use API to find the layout driver directly (Trond Myklebust) - avoid use last found 'ld' (Xiaomeng Tong) - code movement (Xiaomeng Tong)
v1:https://lore.kernel.org/lkml/20220327080230.12134-1-xiam0nd.tong@gmail.com/
--- fs/nfs/callback_proc.c | 32 ++++++++++---------------------- fs/nfs/pnfs.c | 5 +++++ fs/nfs/pnfs.h | 1 + 3 files changed, 16 insertions(+), 22 deletions(-)
diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c index c343666d9a42..579887749870 100644 --- a/fs/nfs/callback_proc.c +++ b/fs/nfs/callback_proc.c @@ -358,39 +358,27 @@ __be32 nfs4_callback_devicenotify(void *argp, void *resp, struct cb_process_state *cps) { struct cb_devicenotifyargs *args = argp; + const struct pnfs_layoutdriver_type *ld; uint32_t i; - __be32 res = 0; - struct nfs_client *clp = cps->clp; - struct nfs_server *server = NULL;
- if (!clp) { - res = cpu_to_be32(NFS4ERR_OP_NOT_IN_SESSION); - goto out; + if (!cps->clp) { + kfree(args->devs); + return cpu_to_be32(NFS4ERR_OP_NOT_IN_SESSION); }
for (i = 0; i < args->ndevs; i++) { struct cb_devicenotifyitem *dev = &args->devs[i];
- if (!server || - server->pnfs_curr_ld->id != dev->cbd_layout_type) { - rcu_read_lock(); - list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link) - if (server->pnfs_curr_ld && - server->pnfs_curr_ld->id == dev->cbd_layout_type) { - rcu_read_unlock(); - goto found; - } - rcu_read_unlock(); - continue; + ld = pnfs_find_layoutdriver(dev->cbd_layout_type); + if (ld) { + nfs4_delete_deviceid(ld, cps->clp, + &dev->cbd_dev_id); + module_put(ld->owner); } - - found: - nfs4_delete_deviceid(server->pnfs_curr_ld, clp, &dev->cbd_dev_id); }
-out: kfree(args->devs); - return res; + return 0; }
/* diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c index 7c9090a28e5c..112c36977feb 100644 --- a/fs/nfs/pnfs.c +++ b/fs/nfs/pnfs.c @@ -92,6 +92,11 @@ find_pnfs_driver(u32 id) return local; }
+const struct pnfs_layoutdriver_type *pnfs_find_layoutdriver(u32 id) +{ + return find_pnfs_driver(id); +} + void unset_pnfs_layoutdriver(struct nfs_server *nfss) { diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h index f4d7548d67b2..873ea8fe945b 100644 --- a/fs/nfs/pnfs.h +++ b/fs/nfs/pnfs.h @@ -234,6 +234,7 @@ struct pnfs_devicelist {
extern int pnfs_register_layoutdriver(struct pnfs_layoutdriver_type *); extern void pnfs_unregister_layoutdriver(struct pnfs_layoutdriver_type *); +extern const struct pnfs_layoutdriver_type *pnfs_find_layoutdriver(u32 id);
/* nfs4proc.c */ extern size_t max_response_pages(struct nfs_server *server);