Bug 7987 - ACL can get lost when files are being renamed
ACL can get lost when files are being renamed
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: File services
3.6.0pre1
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-04 12:28 UTC by Björn Jacke
Modified: 2011-04-14 19:12 UTC (History)
1 user (show)

See Also:


Attachments
0001-s3-Redirect-chmod-in-vfs_acl_-xattr-tdb.patch (2.13 KB, patch)
2011-03-04 12:28 UTC, Björn Jacke
no flags Details
git-am patch for 3.5.next (3.45 KB, patch)
2011-04-06 00:27 UTC, Jeremy Allison
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Jacke 2011-03-04 12:28:40 UTC
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?
Comment 1 Volker Lendecke 2011-03-04 18:21:47 UTC
Just a quick remark: This did solve a customer problem successfully.

Volker
Comment 2 Jeremy Allison 2011-03-07 13:43:16 UTC
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.
Comment 3 Volker Lendecke 2011-03-07 18:53:08 UTC
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
Comment 4 Jeremy Allison 2011-03-07 21:58:44 UTC
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.
Comment 5 Jeremy Allison 2011-04-06 00:27:26 UTC
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 6 Volker Lendecke 2011-04-07 11:23:42 UTC
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
Comment 7 Jeremy Allison 2011-04-07 18:29:45 UTC
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.
Comment 8 Jeremy Allison 2011-04-12 18:52:14 UTC
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.
Comment 9 Karolin Seeger 2011-04-14 19:12:39 UTC
Pushed to v3-5-test.
Closing out bug report.

Thanks!