Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1247538 > unrolled thread

Re: [PATCH v4 09/11] smack: namespace groundwork

Started by"Hillf Danton" <hillf.zj@alibaba-inc.com>
First post2015-10-15 09:50 +0200
Last post2015-10-16 12:20 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 09/11] smack: namespace groundwork "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2015-10-15 09:50 +0200
    Re: [PATCH v4 09/11] smack: namespace groundwork Lukasz Pawelczyk <l.pawelczyk@samsung.com> - 2015-10-15 14:50 +0200
      Re: [PATCH v4 09/11] smack: namespace groundwork Lukasz Pawelczyk <l.pawelczyk@samsung.com> - 2015-10-15 15:00 +0200
        Re: [PATCH v4 09/11] smack: namespace groundwork "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2015-10-16 05:20 +0200
          Re: [PATCH v4 09/11] smack: namespace groundwork Lukasz Pawelczyk <l.pawelczyk@samsung.com> - 2015-10-16 12:20 +0200

#1247538 — Re: [PATCH v4 09/11] smack: namespace groundwork

From"Hillf Danton" <hillf.zj@alibaba-inc.com>
Date2015-10-15 09:50 +0200
SubjectRe: [PATCH v4 09/11] smack: namespace groundwork
Message-ID<qjLsu-8id-13@gated-at.bofh.it>
> 
> +static inline void smack_userns_free(struct user_namespace *ns)
> +{
> +	struct smack_ns *snsp = ns->security;
> +	struct smack_known *skp;
> +	struct smack_known_ns *sknp, *n;
> +
> +	list_for_each_entry_safe(sknp, n, &snsp->smk_mapped, smk_list_ns) {
> +		skp = sknp->smk_unmapped;
> +
> +		mutex_lock(&skp->smk_mapped_lock);
> +		list_del_rcu(&sknp->smk_list_known);
> +		if (sknp->smk_allocated)
> +			kfree(sknp->smk_mapped);
> +		kfree(sknp);

sknp is released,
> +		mutex_unlock(&skp->smk_mapped_lock);
> +
> +		list_del(&sknp->smk_list_ns);

but it is used again! typo?
> +	}
> +
> +	kfree(snsp);
> +}
> +
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1247770

FromLukasz Pawelczyk <l.pawelczyk@samsung.com>
Date2015-10-15 14:50 +0200
Message-ID<qjQ8O-6Gn-11@gated-at.bofh.it>
In reply to#1247538
On czw, 2015-10-15 at 15:38 +0800, Hillf Danton wrote:
> > 
> > +static inline void smack_userns_free(struct user_namespace *ns)
> > +{
> > +	struct smack_ns *snsp = ns->security;
> > +	struct smack_known *skp;
> > +	struct smack_known_ns *sknp, *n;
> > +
> > +	list_for_each_entry_safe(sknp, n, &snsp->smk_mapped,
> > smk_list_ns) {
> > +		skp = sknp->smk_unmapped;
> > +
> > +		mutex_lock(&skp->smk_mapped_lock);
> > +		list_del_rcu(&sknp->smk_list_known);
> > +		if (sknp->smk_allocated)
> > +			kfree(sknp->smk_mapped);
> > +		kfree(sknp);
> 
> sknp is released,
> > +		mutex_unlock(&skp->smk_mapped_lock);
> > +
> > +		list_del(&sknp->smk_list_ns);
> 
> but it is used again! typo?

No, not a typo. A regular bug. Thanks for spotting it. Also sync
mechanism before freeing was missing:

	skp = sknp->smk_unmapped;

	mutex_lock(&skp->smk_mapped_lock);
	list_del_rcu(&sknp->smk_list_known);
	mutex_unlock(&skp->smk_mapped_lock);

	list_del(&sknp->smk_list_ns);

	call_rcu(&sknp->smk_rcu, smk_free_known_ns);


-- 
Lukasz Pawelczyk
Samsung R&D Institute Poland
Samsung Electronics




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1247779

FromLukasz Pawelczyk <l.pawelczyk@samsung.com>
Date2015-10-15 15:00 +0200
Message-ID<qjQiu-6RK-19@gated-at.bofh.it>
In reply to#1247770
On czw, 2015-10-15 at 14:41 +0200, Lukasz Pawelczyk wrote:

> No, not a typo. A regular bug. Thanks for spotting it. Also sync
> mechanism before freeing was missing:


Hitfix, will be integrated with the next respin:

diff --git a/security/smack/smack.h b/security/smack/smack.h
index 3d432f4..3a795bf 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -97,6 +97,7 @@ struct smack_ns {
 struct smack_known_ns {
        struct list_head        smk_list_known;
        struct list_head        smk_list_ns;
+       struct rcu_head         smk_rcu;
        struct user_namespace   *smk_ns;
        char                    *smk_mapped;
        struct smack_known      *smk_unmapped;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 8e0da67..234da71 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4669,6 +4669,15 @@ static inline int smack_userns_create(struct user_namespace *ns)
        return 0;
 }
 
+static void smk_free_known_ns(struct rcu_head *head)
+{
+       struct smack_known_ns *sknp = container_of(head, struct smack_known_ns, smk_rcu);
+
+       if (sknp->smk_allocated)
+               kfree(sknp->smk_mapped);
+       kfree(sknp);
+}
+
 static inline void smack_userns_free(struct user_namespace *ns)
 {
        struct smack_ns *snsp = ns->security;
@@ -4680,12 +4689,11 @@ static inline void smack_userns_free(struct user_namespace *ns)
 
                mutex_lock(&skp->smk_mapped_lock);
                list_del_rcu(&sknp->smk_list_known);
-               if (sknp->smk_allocated)
-                       kfree(sknp->smk_mapped);
-               kfree(sknp);
                mutex_unlock(&skp->smk_mapped_lock);
 
                list_del(&sknp->smk_list_ns);
+
+               call_rcu(&sknp->smk_rcu, smk_free_known_ns);
        }
 
        kfree(snsp);
-- 



-- 
Lukasz Pawelczyk
Samsung R&D Institute Poland
Samsung Electronics




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248315

From"Hillf Danton" <hillf.zj@alibaba-inc.com>
Date2015-10-16 05:20 +0200
Message-ID<qk3IJ-1Sx-5@gated-at.bofh.it>
In reply to#1247779
> On czw, 2015-10-15 at 14:41 +0200, Lukasz Pawelczyk wrote:
> 
> > No, not a typo. A regular bug. Thanks for spotting it. Also sync
> > mechanism before freeing was missing:
> 
> 
> Hitfix, will be integrated with the next respin:
> 
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 3d432f4..3a795bf 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -97,6 +97,7 @@ struct smack_ns {
>  struct smack_known_ns {
>         struct list_head        smk_list_known;
>         struct list_head        smk_list_ns;
> +       struct rcu_head         smk_rcu;
>         struct user_namespace   *smk_ns;
>         char                    *smk_mapped;
>         struct smack_known      *smk_unmapped;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8e0da67..234da71 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4669,6 +4669,15 @@ static inline int smack_userns_create(struct user_namespace *ns)
>         return 0;
>  }
> 
> +static void smk_free_known_ns(struct rcu_head *head)
> +{
> +       struct smack_known_ns *sknp = container_of(head, struct smack_known_ns, smk_rcu);
> +
> +       if (sknp->smk_allocated)
> +               kfree(sknp->smk_mapped);
> +       kfree(sknp);
> +}
> +
>  static inline void smack_userns_free(struct user_namespace *ns)
>  {
>         struct smack_ns *snsp = ns->security;
> @@ -4680,12 +4689,11 @@ static inline void smack_userns_free(struct user_namespace *ns)
> 
>                 mutex_lock(&skp->smk_mapped_lock);
>                 list_del_rcu(&sknp->smk_list_known);
> -               if (sknp->smk_allocated)
> -                       kfree(sknp->smk_mapped);
> -               kfree(sknp);
>                 mutex_unlock(&skp->smk_mapped_lock);
> 
>                 list_del(&sknp->smk_list_ns);

Is list_del safe, given the operation

+	mutex_lock(&snsp->smk_mapped_lock);
+	list_add_rcu(&sknp->smk_list_ns, &snsp->smk_mapped);
+	mutex_unlock(&snsp->smk_mapped_lock);

in smk_import_mapped() function(copied below)?

> +
> +               call_rcu(&sknp->smk_rcu, smk_free_known_ns);
>         }
> 
>         kfree(snsp);
> --
>

+static struct smack_known_ns *smk_import_mapped(struct smack_known *skp,
+						struct user_namespace *ns,
+						const char *string, int len)
+{
+	struct smack_ns *snsp = ns->security;
+	struct smack_known_ns *sknp;
+	char *mapped;
+	bool allocated;
+
+	/* Mapping init_user_ns is against the design and pointless */
+	if (ns == &init_user_ns)
+		return ERR_PTR(-EBADR);
+
+	mapped = smk_parse_smack(string, len, &allocated);
+	if (IS_ERR(mapped))
+		return ERR_CAST(mapped);
+
+	mutex_lock(&skp->smk_mapped_lock);
+
+	/*
+	 * Don't allow one<->many mappings in namespace, rename.
+	 * This code won't get triggered for now as trying to assign
+	 * a duplicate is forbidden in proc_label_map_write().
+	 * Leaving this as this function might be also used elsewhere.
+	 */
+	sknp = smk_find_mapped(skp, ns);
+	if (sknp != NULL) {
+		if (sknp->smk_allocated)
+			kfree(sknp->smk_mapped);
+		sknp->smk_mapped = mapped;
+		sknp->smk_allocated = allocated;
+		goto unlockout;
+	}
+
+	sknp = kzalloc(sizeof(*sknp), GFP_KERNEL);
+	if (sknp == NULL) {
+		sknp = ERR_PTR(-ENOMEM);
+		if (allocated)
+			kfree(mapped);
+		goto unlockout;
+	}
+
+	sknp->smk_ns = ns;
+	sknp->smk_mapped = mapped;
+	sknp->smk_allocated = allocated;
+	sknp->smk_unmapped = skp;
+	list_add_rcu(&sknp->smk_list_known, &skp->smk_mapped);
+
+	mutex_lock(&snsp->smk_mapped_lock);
+	list_add_rcu(&sknp->smk_list_ns, &snsp->smk_mapped);
+	mutex_unlock(&snsp->smk_mapped_lock);
+
+unlockout:
+	mutex_unlock(&skp->smk_mapped_lock);
+
+	return sknp;
+}
 
> 
> 
> --
> Lukasz Pawelczyk
> Samsung R&D Institute Poland
> Samsung Electronics
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248552

FromLukasz Pawelczyk <l.pawelczyk@samsung.com>
Date2015-10-16 12:20 +0200
Message-ID<qkahb-3cA-3@gated-at.bofh.it>
In reply to#1248315
On pią, 2015-10-16 at 11:04 +0800, Hillf Danton wrote:
> > +
> >  static inline void smack_userns_free(struct user_namespace *ns)
> >  {
> >         struct smack_ns *snsp = ns->security;
> > @@ -4680,12 +4689,11 @@ static inline void smack_userns_free(struct
> > user_namespace *ns)
> > 
> >                 mutex_lock(&skp->smk_mapped_lock);
> >                 list_del_rcu(&sknp->smk_list_known);
> > -               if (sknp->smk_allocated)
> > -                       kfree(sknp->smk_mapped);
> > -               kfree(sknp);
> >                 mutex_unlock(&skp->smk_mapped_lock);
> > 
> >                 list_del(&sknp->smk_list_ns);
> 
> Is list_del safe, given the operation
> 
> +	mutex_lock(&snsp->smk_mapped_lock);
> +	list_add_rcu(&sknp->smk_list_ns, &snsp->smk_mapped);
> +	mutex_unlock(&snsp->smk_mapped_lock);
> 
> in smk_import_mapped() function(copied below)?

Yes, the namespace is destroyed when all its references are gone. This
also includes processes that were in that namespace. Meaning there is
no way to import a new mapping for them anymore at this point.


-- 
Lukasz Pawelczyk
Samsung R&D Institute Poland
Samsung Electronics




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web