From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kinglong Mee Subject: Re: [PATCH 6/6 v9] nfsd: Allows user un-mounting filesystem where nfsd exports base on Date: Fri, 28 Aug 2015 07:15:41 +0800 Message-ID: <55DF9A1D.5020209@gmail.com> References: <55D2DBF6.3010406@gmail.com> <55D2DD8F.6070501@gmail.com> <20150819045059.GB18890@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: "J. Bruce Fields" , "linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, NeilBrown , Trond Myklebust , kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org To: Al Viro Return-path: In-Reply-To: <20150819045059.GB18890-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> Sender: linux-nfs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-fsdevel.vger.kernel.org On 8/19/2015 12:50, Al Viro wrote: > On Tue, Aug 18, 2015 at 03:23:59PM +0800, Kinglong Mee wrote: >> @@ -181,7 +191,11 @@ static int expkey_show(struct seq_file *m, >> if (test_bit(CACHE_VALID, &h->flags) && >> !test_bit(CACHE_NEGATIVE, &h->flags)) { >> seq_printf(m, " "); >> - seq_path(m, &ek->ek_path, "\\ \t\n"); >> + if (legitimize_mntget(ek->ek_path.mnt)) { >> + seq_path(m, &ek->ek_path, "\\ \t\n"); >> + mntput(ek->ek_path.mnt); >> + } else >> + seq_printf(m, "Dir-unmounting"); > > IDGI... What locking environment do you have here? Note that use > of mnt_add_count(mnt, -1) in MNT_SYNC_UMOUNT case in __legitimize_mnt() > is OK only because we > a) are under rcu_read_lock() and > b) have synchronize_rcu() in namespace_unlock(). There are not any locking exist is this patch site. Thanks for your comments about the mnt_add_count() following. I want add rcu_read_lock in legitimize_mntget as, struct vfsmount *legitimize_mntget(struct vfsmount *vfsmnt) { rcu_read_lock(); ...... rcu_read_unlock(); return vfsmnt; } Is it OK? > > You don't seem to be under rcu_read_lock() here, so what's to stop that > from racing with the final mntput()? IOW, suppose that on the first > pass through legitimize_mntget() you read mount_lock, bump refcount, > recheck the lock and notice that it has been touched. You proceed to > decrement refcount. Fine, except that what would've been the final > mntput() has just noticed that refcount hadn't reached 0 and buggered > off. And in the meanwhile, MNT_UMOUNT had been set. Now you decrement > the refcount to zero, notice that MNT_UMOUNT and go away. Have a nice > leak... > > The only reason why we are able to get away with mnt_add_count(mnt, -1) in > that specific case in legitimize_mnt() is that MNT_SYNC_UMOUNT must have > been set after we'd got rcu_read_lock() (otherwise we would've either hit > a mismatch on mount_lock before incrementing refcount or wouldn't have > run into that vfsmount at all) and thus we _know_ that the process that > has set MNT_SYNC_UMOUNT couldn't have passed through synchronize_rcu() > in namespace_unlock(), so it couldn't reach mntput_no_expire() until our > caller does rcu_read_unlock() and we are free to decrement the refcount and > leave - we won't be dropping the last reference here. > > Without MNT_SYNC_UMOUNT the callers of __legitimize_mnt() must use mntput() > to drop the mistakenly acquired reference. Exactly because they can't > rely on that syncronize_rcu() delaying the final mntput(). > >> @@ -664,7 +696,12 @@ static int svc_export_show(struct seq_file *m, >> return 0; >> } >> exp = container_of(h, struct svc_export, h); >> - seq_path(m, &exp->ex_path, " \t\n\\"); >> + if (legitimize_mntget(exp->ex_path.mnt)) { >> + seq_path(m, &exp->ex_path, " \t\n\\"); >> + mntput(exp->ex_path.mnt); >> + } else >> + seq_printf(m, "Dir-unmounting"); > > Ditto. And grabbing/dropping references here seems to be an overkill... Do you mean that calling seq_path without legitimize_mntget here ? But the mnt is without reference to vfsmnt, only a fs_pin many times. > >> @@ -819,6 +867,12 @@ exp_find_key(struct cache_detail *cd, struct auth_domain *clp, int fsid_type, >> err = cache_check(cd, &ek->h, reqp); >> if (err) >> return ERR_PTR(err); >> + >> + if (!legitimize_mntget(ek->ek_path.mnt)) { >> + cache_put(&ek->h, ek->cd); >> + return ERR_PTR(-ENOENT); >> + } > > Ditto. > >> @@ -842,6 +896,8 @@ exp_get_by_name(struct cache_detail *cd, struct auth_domain *clp, >> err = cache_check(cd, &exp->h, reqp); >> if (err) >> return ERR_PTR(err); >> + >> + mntget(exp->ex_path.mnt); > > What's to make that mntget() legitimate? The mnt has the reference to vfsmnt here, so just using mntget is safe. > >> static inline struct svc_export *exp_get(struct svc_export *exp) >> { >> cache_get(&exp->h); >> + mntget(exp->ex_path.mnt); > > Ditto. > The mnt has the reference, Same as above. thanks, Kinglong Mee -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html