Created attachment 6278 [details] 0001-s3-Redirect-chmod-in-vfs_acl_-xattr-tdb.patch attached is the patch fixes a situation where a ACL got lost upon rename. Jeremy, can you please have a look?
Just a quick remark: This did solve a customer problem successfully. Volker
I'm sure the effect is right - but I'm not 100% happy about calling down to the direct POSIX API without going through the VFS on a higher level module. Can you explain exactly what the codepaths were through the rename that caused this to happen ? I can see why calling the chain: vfswrap_chmod->vfswrap_chmod_acl->chmod_acl->copy_access_posix_acl->SMB_VFS_SYS_ACL_SET_FILE would mess up the underlying xattr or tdb-stored ACL, but I'm wondering if we can do something a little more clever here than simply calling chmod() directly. It might be that I need to re-visit the relationship between the mask changed by the chmod and the underlying POSIX ACL to fix this "correctly" going forward. Jeremy.
On Mon, Mar 07, 2011 at 01:43:17PM +0000, samba-bugs@samba.org wrote: > Can you explain exactly what the codepaths were through the rename that caused > this to happen ? I can see why calling the chain: To be honest, I don't really remember. Just put a breakpoint there. > > vfswrap_chmod->vfswrap_chmod_acl->chmod_acl->copy_access_posix_acl->SMB_VFS_SYS_ACL_SET_FILE > > would mess up the underlying xattr or tdb-stored ACL, but I'm wondering if we > can do something a little more clever here than simply calling chmod() > directly. > > It might be that I need to re-visit the relationship between the mask changed > by the chmod and the underlying POSIX ACL to fix this "correctly" going > forward. I think it's just the combination of both ACL worlds that kill us here. And to be honest, doing the SMB_VFS_CHMOD_ACL magic inside vfs_default looks like the wrong layering to me. For my taste, that's too much magic behind the seemingly simple last module in the whole chain. My 2ct. Volker
I agree (about the wrong layering). It's the last holdover from the old POSIX ACL as default ACL model that Michael fixed everywhere else. I'll look at restructuring that. So a simple smbclient rename triggers it, or does it require a Windows client rename ? I don't have a portable Windows env. here in the UK. Jeremy.
Created attachment 6377 [details] git-am patch for 3.5.next I think this is the correct fix. Can you review please ? There is no reason for smbd with Windows ACLs to use chmod or fchmod unless it's a file opened with UNIX extensions or with posix pathnames. Jeremy.
Comment on attachment 6377 [details] git-am patch for 3.5.next Question: Does that also fix the same thing if coming in from cifsfs? Volker
No, if cifsfs does a Windows-style SetACL then we'll lose it on rename from a CIFSFS client using posix pathnames. But I'd argue that cifsfs should probably be using POSIX ACL set in this case (IMHO). Jeremy.
Ok, I've looking into this and the patch does fix the rename problem from cifsfs. If cifsfs negotiates POSIX pathnames the problematic setting of the archive bit is explicitly not done, and if cifsfs doesn't negotiate POSIX pathnames (acting identically to a Windows client) then the patch ensures the chmod is ignored. There is one outstanding issue where a cifsfs client negotiates POSIX pathnames, then proceeds to use Windows style ACL set calls, not POSIX acl set calls. I don't think any versions of the current Linux cifsfs client does this. I'd also argue this is out of scope for this specific bug fix. So right now I'd recommend this be adopted for 3.5.next. Jeremy.
Pushed to v3-5-test. Closing out bug report. Thanks!