On Tue, Sep 01, 2020 at 08:39:55AM -0400, Stephen Smalley wrote:
> On Tue, Sep 1, 2020 at 7:59 AM liwugang <liwugang@xxxxxxx> wrote:
> >
> > The hash of each directory should be determined by the file contexts of
> > the current directory and subdirectories, but the existing logic also
> > includes the ancestor directories. The first optimization is to exclude
> > them. So it should be break When the first complete match found in function
> > lookup_all.
> >
> > If the current directory has corresponding file contexts and subdirectories
> > have not, subdirectories will use the current direcorty's. There is no need
> > to calculate the hash for the subdirectories. It will save time, espacially
> > for user data(/data/media/0/). The second optimization is not to check the
> > hash of the subdirectories.
> >
> > Example:
> > /data/(.*)? u:object_r:system_data_file:s0
> > /data/backup(/.*)? u:object_r:backup_data_file:s0
> >
> > If the file context of "/data/(.*)?" changes and "/data/backup(/.*)?" does not
> > change, directory(/data/backup) and the subdirectories will restorecon because of
> > hash changed. But actually there is no need the restorecon.
> >
> > Signed-off-by: liwugang <liwugang@xxxxxxx>
> > ---
> > libselinux/include/selinux/label.h | 5 +--
> > libselinux/src/label.c | 11 +++---
> > libselinux/src/label_file.c | 17 +++++++---
> > libselinux/src/label_internal.h | 6 ++--
> > libselinux/src/selinux_restorecon.c | 34 ++++++++++++++-----
> > .../selabel_get_digests_all_partial_matches.c | 3 +-
> > 6 files changed, 55 insertions(+), 21 deletions(-)
> >
> > diff --git a/libselinux/include/selinux/label.h b/libselinux/include/selinux/label.h
> > index e8983606..d91ceb6f 100644
> > --- a/libselinux/include/selinux/label.h
> > +++ b/libselinux/include/selinux/label.h
> > @@ -110,9 +110,10 @@ extern bool selabel_get_digests_all_partial_matches(struct selabel_handle *rec,
> > const char *key,
> > uint8_t **calculated_digest,
> > uint8_t **xattr_digest,
> > - size_t *digest_len);
> > + size_t *digest_len,
> > + size_t *num_matches);
> > extern bool selabel_hash_all_partial_matches(struct selabel_handle *rec,
> > - const char *key, uint8_t* digest);
> > + const char *key, uint8_t* digest, size_t *num_matches);
> >
> > extern int selabel_lookup_best_match(struct selabel_handle *rec, char **con,
> > const char *key, const char **aliases, int type);
>
> This is a public API and a stable ABI for libselinux, so you cannot
> make incompatible changes to it.
> You would need to introduce a new API with the extended interface.
>
OK, I will add new API.
> > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> > index 6eeeea68..c99eb251 100644
> > --- a/libselinux/src/label_file.c
> > +++ b/libselinux/src/label_file.c
> > @@ -955,7 +955,10 @@ static const struct spec **lookup_all(struct selabel_handle *rec,
> > if (match_count) {
> > result[*match_count] = spec;
> > *match_count += 1;
> > - // Continue to find all the matches.
> > + if (rc == REGEX_MATCH) {
> > + break;
> > + }
> > + // Continue to find the matches until the first full match found.
>
> I'm not sure this works the way you intend. /data/(.*)? is a full
> match for /data/backup. Do you want to stop there and not include
> /data/backup(/.*)? This also changes behavior of an existing API/ABI
> in an incompatible manner.
>
My original intention is that /data/backup(/.*)? is always after /data/(.*)?,
traversing from back to front, The /data/backup(/.*)? will first be meet
the condition. But after checking the code, the function sort_specs don't
sort the entries. just put the entries with the meta characters in the front.
So it can't guarantee the sequence I want.
I think I also need add the function to sort the entries.
> > diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
> > index 6993be6f..417b619c 100644
> > --- a/libselinux/src/selinux_restorecon.c
> > +++ b/libselinux/src/selinux_restorecon.c
>
> Last I looked, Android wasn't using the upstream selinux_restorecon
> (which was actually a back-port / adaptation of Android's
> selinux_android_restorecon to upstream) at all, so any changes here
> won't actually affect Android relabeling AFAIK.
Yes, I think upstream will also benefit from this patch.
Thanks very much to review the patch.
