From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1424619AbcFHQKx (ORCPT ); Wed, 8 Jun 2016 12:10:53 -0400 Received: from linuxhacker.ru ([217.76.32.60]:57104 "EHLO fiona.linuxhacker.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1423156AbcFHQKw convert rfc822-to-8bit (ORCPT ); Wed, 8 Jun 2016 12:10:52 -0400 Subject: Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Mime-Version: 1.0 (Apple Message framework v1283) Content-Type: text/plain; charset=us-ascii From: Oleg Drokin In-Reply-To: <1465383501.27742.19.camel@poochiereds.net> Date: Wed, 8 Jun 2016 12:10:40 -0400 Cc: "J. Bruce Fields" , linux-nfs@vger.kernel.org, " Mailing List" Content-Transfer-Encoding: 8BIT Message-Id: References: <4EDA6CFD-1FE8-4FCA-ACCF-84250BE342CB@linuxhacker.ru> <1465319435.3024.25.camel@poochiereds.net> <0F21EDD6-5CBB-4B5B-A1FF-E066011D18D6@linuxhacker.ru> <1465329897.3024.38.camel@poochiereds.net> <752F7196-1EE7-4FB3-8769-177131C8A793@linuxhacker.ru> <1465344205.3024.42.camel@poochiereds.net> <1465383501.27742.19.camel@poochiereds.net> To: Jeff Layton X-Mailer: Apple Mail (2.1283) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Jun 8, 2016, at 6:58 AM, Jeff Layton wrote: > A simple way to confirm that might be to convert all of the read locks > on the st_rwsem to write locks. That will serialize all of the open > operations and should prevent that particular race from occurring. > > If that works, we'd probably want to fix it in a less heavy-handed way, > but I'd have to think about how best to do that. So I looked at the call sites for nfs4_get_vfs_file(), how about something like this: after we grab the fp->fi_lock, we can do test_access(open->op_share_access, stp); If that returns true - just drop the spinlock and return EAGAIN. The callsite in nfs4_upgrade_open() would handle that by retesting the access map again and either coming back in or more likely reusing the now updated stateid (synchronised by the fi_lock again). We probably need to convert the whole access map testing there to be under fi_lock. Something like: nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *cur_fh, struct nfs4_ol_stateid *stp, struct nfsd4_open *open) { __be32 status; unsigned char old_deny_bmap = stp->st_deny_bmap; again: + spin_lock(&fp->fi_lock); if (!test_access(open->op_share_access, stp)) { + spin_unlock(&fp->fi_lock); + status = nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open); + if (status == -EAGAIN) + goto again; + return status; + } /* test and set deny mode */ - spin_lock(&fp->fi_lock); status = nfs4_file_check_deny(fp, open->op_share_deny); The call in nfsd4_process_open2() I think cannot hit this condition, right? probably can add a WARN_ON there? BUG_ON? more sensible approach? Alternatively we can probably always call nfs4_get_vfs_file() under this spinlock, just have it drop that for the open and then reobtain (already done), not as transparent I guess. Or the fi_lock might be converted to say a mutex, so we can sleep with it held and then we can hold it across whole invocation of nfs4_get_vfs_file() and access testing and stuff.