Hi, I am having a problem with ACLs in NFS Ganesha v2.6.3. Based on what I believe to be the cause of the problem, I think the problem will exist in the tip too. I am using a proprietary fsal. The symptom of the problem is simple. If I create a file and quickly read the ACLs, nothing is returned: # echo test > test.txt # nfs4_getfacl test.txt # If I restart ganesha, I see the correct ACLs: # nfs4_getfacl test.txt A::OWNER@:rwatTnNcCy A::GROUP@:rtncy A::EVERYONE@:rtncy Also, if I create a file, and wait over 60 seconds, then I can read the correct ACLs. This is all working fine in v2.5.1. I think I see the problem in the code. In my case, the nfs client first sends a GETATTR request with the ACL bit set in the request mask. But then it sends another one without the ACL bit set. During this second request, when ACLs are not requested, the entry in mdcache is updated in mdcache_refresh_attrs. In 2.5.1, mdcache_refresh_attrs, after the subfsal is called to get the attributes, it does: if (entry->attrs.acl != NULL) { /* We used to have an ACL... */ if (need_acl) { /* We requested update of an existing ACL, release the * old one. */ nfs4_acl_release_entry(entry->attrs.acl); } else { /* The ACL wasn't requested, move it into the * new attributes so we will retain it and make * it such that the entry attrs DO request the * ACL. */ attrs.acl = entry->attrs.acl; attrs.valid_mask |= ATTR_ACL; entry->attrs.request_mask |= ATTR_ACL; } /* ACL was released or moved to new attributes. */ entry->attrs.acl = NULL; } Here, the second path is taken. Basically it moves the ACLs to attrs, but it sets the entry's request mask bit. Soon after, fsal_copy_attrs is called which copies the ACLs back to the entry. It does this because the entry's request mask ACL bit is set. If it was not set, the ACLs would not be copied. Unfortunately, this is not happening in 2.6.3: In 2.6.3, this functionality was moved into mdc_update_attr_cache: if (entry->attrs.acl != NULL) { /* We used to have an ACL... */ if (attrs->acl != NULL) { /* We got an ACL from the sub FSAL whether we asked for * it or not, given that we had an ACL before, and we * got a new one, update the ACL, so release the old * one. */ nfs4_acl_release_entry(entry->attrs.acl); } else { /* A new ACL wasn't provided, so move the old one * into the new attributes so it will be preserved * by the fsal_copy_attrs. */ attrs->acl = entry->attrs.acl; attrs->valid_mask |= ATTR_ACL; } /* NOTE: Because we already had an ACL, * entry->attrs.request_mask MUST have the ATTR_ACL bit set. * This will assure that fsal_copy_attrs below will copy the * selected ACL (old or new) into entry->attrs. */ /* ACL was released or moved to new attributes. */ entry->attrs.acl = NULL; } else if (attrs->acl != NULL) { /* We didn't have an ACL before, but we got a new one. We may * not have asked for it, but receive it anyway. */ entry->attrs.request_mask |= ATTR_ACL; } We are taking the path where it says "A new ACL wasn't provided". It is moving the existing ACL into attrs, which we would like to be copied later by fsal_copy_attrs, just like in 2.5.1. But note in 2.6, we are never setting the ACL bit in the entry's request mask, only in attrs. Therefore, fsal_copy_attrs will not copy the ACL back to the entry, and the ACL will be lost forever. Even worse, is the trusted ACL flag in mde_flags is set, so now the caller thinks there are no ACLs at all! The logic looks the same in the current 2.8 code to me. Please let me know if my analysis is correct or not, and if so, if there is any possible workaround. Bill
I think this should already work. As the comment there says, if the entry has an ACL, it's request_mask should have ATTR_ACL set. Looking through the code, it looks like the acl ptr and ATTR_ACL are always set together. Daniel On 3/1/19 6:34 PM, katcherw@gmail.com wrote:
Hi,
I am having a problem with ACLs in NFS Ganesha v2.6.3. Based on what I believe to be the cause of the problem, I think the problem will exist in the tip too. I am using a proprietary fsal.
The symptom of the problem is simple. If I create a file and quickly read the ACLs, nothing is returned:
# echo test > test.txt # nfs4_getfacl test.txt #
If I restart ganesha, I see the correct ACLs:
# nfs4_getfacl test.txt A::OWNER@:rwatTnNcCy A::GROUP@:rtncy A::EVERYONE@:rtncy
Also, if I create a file, and wait over 60 seconds, then I can read the correct ACLs.
This is all working fine in v2.5.1.
I think I see the problem in the code. In my case, the nfs client first sends a GETATTR request with the ACL bit set in the request mask. But then it sends another one without the ACL bit set. During this second request, when ACLs are not requested, the entry in mdcache is updated in mdcache_refresh_attrs.
In 2.5.1, mdcache_refresh_attrs, after the subfsal is called to get the attributes, it does:
if (entry->attrs.acl != NULL) { /* We used to have an ACL... */ if (need_acl) { /* We requested update of an existing ACL, release the * old one. */ nfs4_acl_release_entry(entry->attrs.acl); } else { /* The ACL wasn't requested, move it into the * new attributes so we will retain it and make * it such that the entry attrs DO request the * ACL. */ attrs.acl = entry->attrs.acl; attrs.valid_mask |= ATTR_ACL; entry->attrs.request_mask |= ATTR_ACL; }
/* ACL was released or moved to new attributes. */ entry->attrs.acl = NULL; }
Here, the second path is taken. Basically it moves the ACLs to attrs, but it sets the entry's request mask bit. Soon after, fsal_copy_attrs is called which copies the ACLs back to the entry. It does this because the entry's request mask ACL bit is set. If it was not set, the ACLs would not be copied.
Unfortunately, this is not happening in 2.6.3:
In 2.6.3, this functionality was moved into mdc_update_attr_cache:
if (entry->attrs.acl != NULL) { /* We used to have an ACL... */ if (attrs->acl != NULL) { /* We got an ACL from the sub FSAL whether we asked for * it or not, given that we had an ACL before, and we * got a new one, update the ACL, so release the old * one. */ nfs4_acl_release_entry(entry->attrs.acl); } else { /* A new ACL wasn't provided, so move the old one * into the new attributes so it will be preserved * by the fsal_copy_attrs. */ attrs->acl = entry->attrs.acl; attrs->valid_mask |= ATTR_ACL; }
/* NOTE: Because we already had an ACL, * entry->attrs.request_mask MUST have the ATTR_ACL bit set. * This will assure that fsal_copy_attrs below will copy the * selected ACL (old or new) into entry->attrs. */
/* ACL was released or moved to new attributes. */ entry->attrs.acl = NULL; } else if (attrs->acl != NULL) { /* We didn't have an ACL before, but we got a new one. We may * not have asked for it, but receive it anyway. */ entry->attrs.request_mask |= ATTR_ACL; }
We are taking the path where it says "A new ACL wasn't provided". It is moving the existing ACL into attrs, which we would like to be copied later by fsal_copy_attrs, just like in 2.5.1. But note in 2.6, we are never setting the ACL bit in the entry's request mask, only in attrs. Therefore, fsal_copy_attrs will not copy the ACL back to the entry, and the ACL will be lost forever. Even worse, is the trusted ACL flag in mde_flags is set, so now the caller thinks there are no ACLs at all!
The logic looks the same in the current 2.8 code to me.
Please let me know if my analysis is correct or not, and if so, if there is any possible workaround.
Bill _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
Daniel, I don't think this assumption is always correct. In mdcache_refresh_attrs: /* We will want all the requested attributes in the entry */ entry->attrs.request_mask = attrs.request_mask; In my situation, this is turning off the ATTR_ACL bit in the entry's request mask. Bill
On 3/4/19 1:52 PM, katcherw@gmail.com wrote:
Daniel,
I don't think this assumption is always correct. In mdcache_refresh_attrs:
/* We will want all the requested attributes in the entry */ entry->attrs.request_mask = attrs.request_mask;
In my situation, this is turning off the ATTR_ACL bit in the entry's request mask.
Okay, I see. Something like this: https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/446980 Daniel
On 3/4/19 1:52 PM, katcherw@gmail.com wrote:
Daniel,
I don't think this assumption is always correct. In mdcache_refresh_attrs:
/* We will want all the requested attributes in the entry */ entry->attrs.request_mask = attrs.request_mask;
In my situation, this is turning off the ATTR_ACL bit in the entry's request mask.
Okay, I see. Something like this:
I've been looking over the code. That simple fix might be enough, but I'm not sure. I think our attribute tracking has gotten overly complex and some things are disconnected. One thought I have is for the time stamp of an attribute to actually be supplied by the FSAL, at least conceptually (i.e. we could easily set it in mdcache_refresh_attrs just after calling FSAL getattrs). But make the time stamps be part of struct attrlist. Then each attribute (or group of attributes) has a bit in valid_mask, and a time stamp. When the cached attributes are updated, the bit in the valid mask, the attribute(s), and the time stamp are all copied together. I wonder if we really need separate trust flags in mde_flags? And we should make it such that request_mask is ONLY used to indicate to getattrs which attributes are of interest (and of course mdcache may pass a different request_mask to the underlying FSAL than it was passed). In the current code, I see that fs_locations_time isn't ever checked, and there is no separate sec_label_time... Frank
On 3/4/19 4:02 PM, Frank Filz wrote:
On 3/4/19 1:52 PM, katcherw@gmail.com wrote:
Daniel,
I don't think this assumption is always correct. In mdcache_refresh_attrs:
/* We will want all the requested attributes in the entry */ entry->attrs.request_mask = attrs.request_mask;
In my situation, this is turning off the ATTR_ACL bit in the entry's request mask.
Okay, I see. Something like this:
I've been looking over the code. That simple fix might be enough, but I'm not sure.
I think our attribute tracking has gotten overly complex and some things are disconnected.
One thought I have is for the time stamp of an attribute to actually be supplied by the FSAL, at least conceptually (i.e. we could easily set it in mdcache_refresh_attrs just after calling FSAL getattrs). But make the time stamps be part of struct attrlist.
Then each attribute (or group of attributes) has a bit in valid_mask, and a time stamp. When the cached attributes are updated, the bit in the valid mask, the attribute(s), and the time stamp are all copied together.
I wonder if we really need separate trust flags in mde_flags?
And we should make it such that request_mask is ONLY used to indicate to getattrs which attributes are of interest (and of course mdcache may pass a different request_mask to the underlying FSAL than it was passed).
In the current code, I see that fs_locations_time isn't ever checked, and there is no separate sec_label_time...
Frank
I'm not adverse to changing how we handle attribute validity. However, I think a timestamp-per-attribute would more than double the size of the attrlist, and so I think something else is necessary. It would also lead to a *ton* of timestamp comparisons for any getattr operation, which would add to latency. Daniel
On 3/4/19 4:02 PM, Frank Filz wrote:
On 3/4/19 1:52 PM, katcherw@gmail.com wrote:
Daniel,
I don't think this assumption is always correct. In mdcache_refresh_attrs:
/* We will want all the requested attributes in the entry */ entry->attrs.request_mask = attrs.request_mask;
In my situation, this is turning off the ATTR_ACL bit in the entry's request mask.
Okay, I see. Something like this:
I've been looking over the code. That simple fix might be enough, but I'm not sure.
I think our attribute tracking has gotten overly complex and some things are disconnected.
One thought I have is for the time stamp of an attribute to actually be supplied by the FSAL, at least conceptually (i.e. we could easily set it in mdcache_refresh_attrs just after calling FSAL getattrs). But make the time stamps be part of struct attrlist.
Then each attribute (or group of attributes) has a bit in valid_mask, and a time stamp. When the cached attributes are updated, the bit in the valid mask, the attribute(s), and the time stamp are all copied together.
I wonder if we really need separate trust flags in mde_flags?
And we should make it such that request_mask is ONLY used to indicate to getattrs which attributes are of interest (and of course mdcache may pass a different request_mask to the underlying FSAL than it was passed).
In the current code, I see that fs_locations_time isn't ever checked, and there is no separate sec_label_time...
Frank
I'm not adverse to changing how we handle attribute validity. However, I think a timestamp-per-attribute would more than double the size of the attrlist, and so I think something else is necessary. It would also lead to a *ton* of timestamp comparisons for any getattr operation, which would add to latency.
That's why I said "or group of attributes". In fact, other than adding a time stamp for security label, I'm not proposing adding any more time stamps (though I don't know if there might be a case for dividing the POSIX attributes out that are updated by things like layout commit, the question there would be do clients ever request a subset of attributes that are easily maintained by Ganesha without having to go to the file system. Outside of that, all the traditional POSIX attributes need only be covered by a single time stamp. Hmm, another place that might call for more granularity would be really using STATX to request subsets of the POSIX attributes. These ideas would need to be explored in more detail. Frank
Yes, I think that will work. I compiled in the patch and it so far looks like it solved my problem. Thanks! Bill
There are unlikely to be any more 2.6 releases. Daniel On 3/6/19 10:04 AM, katcherw@gmail.com wrote:
Is there any chance of this being backported to the 2.6 branch?
Bill _______________________________________________ Devel mailing list -- devel@lists.nfs-ganesha.org To unsubscribe send an email to devel-leave@lists.nfs-ganesha.org
participants (3)
-
Daniel Gryniewicz -
Frank Filz -
katcherw＠gmail.com