Am Mittwoch, den 21.03.2018, 17:35 -0400 schrieb J . Bruce Fields: > On Wed, Mar 21, 2018 at 08:52:42PM +0100, Lucas Stach wrote: > > When the server is serving a mixed set of clients where some support the > > NFS 4.2 umask attribute and some don't, we need to make sure to reset the > > nfd thread umask to 0 when serving a client that isn't providing the umask, > > otherwise a stale umask from a previous request will be applied. > > > > This was only done in the nfsd4_decode_open() callsite, but not in > > nfsd4_decode_create() leading to newly created directories ending up with > > wrong permissions in some cases. To avoid any such inconsistency in the > > future, reset the umask in nfsd4_decode_fattr() where we know if the > > client did provide a umask. > > Thanks for catching this! (Is it because you were seeing directories > created with incorrect permissions in production?) Yes, we have hit this in our internal infrastructure. > If the next rpc handled by this thread doesn't include a file attribute, > or isn't an NFSv4 request (maybe it's NFSv3), then it won't hit this > code and will still use the non-zero umask. The thread umask seem to be only relevant for the open/create RPC calls, but you are right about the NFS3 case. I didn't think of that. > I was thinking of initializing it in nfsd_setuser, or maybe on each pass > through the loop that processes each op of a compound in > nfsd4_proc_compound.... But I think that would clear umask too > often--it'd clear the umask before it was actually used. > > Actually I don't think there's any correct time to clear the umask, the > problem is where we're setting it. > > We decode the whole compound in one pass, then process each op of an > NFSv4 compound. That means the last op containing a set of the umask > will determine the umask used for the whole compound. That seems wrong. > > I mean, in practice no real client sends compounds with multiple create > operations, so maybe this is all academic, but still I think the correct > thing to do is stop setting current->fs->umask in the xdr decoder and > instead pass the umask value in the compound op arguments and set it > later in the proc code. Yes, this seems like a better way to deal with this. I'll respin this patch. Thanks, Lucas -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
