diff mbox series

[SRU,B,X] xfrm: policy: match with both mark and mask on user interfaces

Message ID 20200810165545.31610-1-stefan.bader@canonical.com
State New
Headers show
Series [SRU,B,X] xfrm: policy: match with both mark and mask on user interfaces | expand

Commit Message

Stefan Bader Aug. 10, 2020, 4:55 p.m. UTC
From: Xin Long <lucien.xin@gmail.com>

In commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"),
it would take 'priority' to make a policy unique, and allow duplicated
policies with different 'priority' to be added, which is not expected
by userland, as Tobias reported in strongswan.

To fix this duplicated policies issue, and also fix the issue in
commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"),
when doing add/del/get/update on user interfaces, this patch is to change
to look up a policy with both mark and mask by doing:

  mark.v == pol->mark.v && mark.m == pol->mark.m

and leave the check:

  (mark & pol->mark.m) == pol->mark.v

for tx/rx path only.

As the userland expects an exact mark and mask match to manage policies.

v1->v2:
  - make xfrm_policy_mark_match inline and fix the changelog as
    Tobias suggested.

Fixes: 295fae568885 ("xfrm: Allow user space manipulation of SPD mark")
Fixes: ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list")
Reported-by: Tobias Brunner <tobias@strongswan.org>
Tested-by: Tobias Brunner <tobias@strongswan.org>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

BugLink: https://bugs.launchpad.net/bugs/1890796

(backported from commit 4f47e8ab6ab796b5380f74866fa5287aca4dcc58)
[smb: work around missing if_id parameter and __xfrm_policy_bysel_ctx]
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---

This is my backport (it did compile ok on Bionic but if someone could
double check this is looking sane) of the upstream change. The two
functions primarily touched have one argument less there. It appeared to
apply with some offset to Xenial as well but I have not compiled it
there.

-Stefan


 include/net/xfrm.h     |  8 +++++---
 net/key/af_key.c       |  4 ++--
 net/xfrm/xfrm_policy.c | 29 +++++++++++++----------------
 net/xfrm/xfrm_user.c   | 16 ++++++++++------
 4 files changed, 30 insertions(+), 27 deletions(-)

Comments

Colin King Aug. 10, 2020, 5:06 p.m. UTC | #1
On 10/08/2020 17:55, Stefan Bader wrote:
> From: Xin Long <lucien.xin@gmail.com>
> 
> In commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"),
> it would take 'priority' to make a policy unique, and allow duplicated
> policies with different 'priority' to be added, which is not expected
> by userland, as Tobias reported in strongswan.
> 
> To fix this duplicated policies issue, and also fix the issue in
> commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"),
> when doing add/del/get/update on user interfaces, this patch is to change
> to look up a policy with both mark and mask by doing:
> 
>   mark.v == pol->mark.v && mark.m == pol->mark.m
> 
> and leave the check:
> 
>   (mark & pol->mark.m) == pol->mark.v
> 
> for tx/rx path only.
> 
> As the userland expects an exact mark and mask match to manage policies.
> 
> v1->v2:
>   - make xfrm_policy_mark_match inline and fix the changelog as
>     Tobias suggested.
> 
> Fixes: 295fae568885 ("xfrm: Allow user space manipulation of SPD mark")
> Fixes: ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list")
> Reported-by: Tobias Brunner <tobias@strongswan.org>
> Tested-by: Tobias Brunner <tobias@strongswan.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1890796
> 
> (backported from commit 4f47e8ab6ab796b5380f74866fa5287aca4dcc58)
> [smb: work around missing if_id parameter and __xfrm_policy_bysel_ctx]
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
> 
> This is my backport (it did compile ok on Bionic but if someone could
> double check this is looking sane) of the upstream change. The two
> functions primarily touched have one argument less there. It appeared to
> apply with some offset to Xenial as well but I have not compiled it
> there.
> 
> -Stefan
> 
> 
>  include/net/xfrm.h     |  8 +++++---
>  net/key/af_key.c       |  4 ++--
>  net/xfrm/xfrm_policy.c | 29 +++++++++++++----------------
>  net/xfrm/xfrm_user.c   | 16 ++++++++++------
>  4 files changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 83eb033f2cc6..b930c5c8e3d3 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -1674,13 +1674,15 @@ int xfrm_policy_walk(struct net *net, struct xfrm_policy_walk *walk,
>  		     void *);
>  void xfrm_policy_walk_done(struct xfrm_policy_walk *walk, struct net *net);
>  int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl);
> -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark,
> +struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net,
> +					  const struct xfrm_mark *mark,
>  					  u8 type, int dir,
>  					  struct xfrm_selector *sel,
>  					  struct xfrm_sec_ctx *ctx, int delete,
>  					  int *err);
> -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8, int dir,
> -				     u32 id, int delete, int *err);
> +struct xfrm_policy *xfrm_policy_byid(struct net *net,
> +				     const struct xfrm_mark *mark, u8,
> +				     int dir, u32 id, int delete, int *err);
>  int xfrm_policy_flush(struct net *net, u8 type, bool task_valid);
>  void xfrm_policy_hash_rebuild(struct net *net);
>  u32 xfrm_get_acqseq(void);
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index d0561c48234e..c8953fe2cfaa 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -2406,7 +2406,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa
>  			return err;
>  	}
>  
> -	xp = xfrm_policy_bysel_ctx(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN,
> +	xp = xfrm_policy_bysel_ctx(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN,
>  				   pol->sadb_x_policy_dir - 1, &sel, pol_ctx,
>  				   1, &err);
>  	security_xfrm_policy_free(pol_ctx);
> @@ -2657,7 +2657,7 @@ static int pfkey_spdget(struct sock *sk, struct sk_buff *skb, const struct sadb_
>  		return -EINVAL;
>  
>  	delete = (hdr->sadb_msg_type == SADB_X_SPDDELETE2);
> -	xp = xfrm_policy_byid(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN,
> +	xp = xfrm_policy_byid(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN,
>  			      dir, pol->sadb_x_policy_id, delete, &err);
>  	if (xp == NULL)
>  		return -ENOENT;
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 3dc1840c81b1..fb9a086395fe 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -718,14 +718,10 @@ static void xfrm_policy_requeue(struct xfrm_policy *old,
>  	spin_unlock_bh(&pq->hold_queue.lock);
>  }
>  
> -static bool xfrm_policy_mark_match(struct xfrm_policy *policy,
> -				   struct xfrm_policy *pol)
> +static inline bool xfrm_policy_mark_match(const struct xfrm_mark *mark,
> +					  struct xfrm_policy *pol)
>  {
> -	if (policy->mark.v == pol->mark.v &&
> -	    policy->priority == pol->priority)
> -		return true;
> -
> -	return false;
> +	return mark->v == pol->mark.v && mark->m == pol->mark.m;
>  }
>  
>  int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
> @@ -743,7 +739,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
>  	hlist_for_each_entry(pol, chain, bydst) {
>  		if (pol->type == policy->type &&
>  		    !selector_cmp(&pol->selector, &policy->selector) &&
> -		    xfrm_policy_mark_match(policy, pol) &&
> +		    xfrm_policy_mark_match(&policy->mark, pol) &&
>  		    xfrm_sec_ctx_match(pol->security, policy->security) &&
>  		    !WARN_ON(delpol)) {
>  			if (excl) {
> @@ -793,10 +789,10 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
>  }
>  EXPORT_SYMBOL(xfrm_policy_insert);
>  
> -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
> -					  int dir, struct xfrm_selector *sel,
> -					  struct xfrm_sec_ctx *ctx, int delete,
> -					  int *err)
> +struct xfrm_policy *
> +xfrm_policy_bysel_ctx(struct net *net, const struct xfrm_mark *mark, u8 type,
> +		      int dir, struct xfrm_selector *sel,
> +		      struct xfrm_sec_ctx *ctx, int delete, int *err)
>  {
>  	struct xfrm_policy *pol, *ret;
>  	struct hlist_head *chain;
> @@ -807,7 +803,7 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
>  	ret = NULL;
>  	hlist_for_each_entry(pol, chain, bydst) {
>  		if (pol->type == type &&
> -		    (mark & pol->mark.m) == pol->mark.v &&
> +		    xfrm_policy_mark_match(mark, pol) &&
>  		    !selector_cmp(sel, &pol->selector) &&
>  		    xfrm_sec_ctx_match(ctx, pol->security)) {
>  			xfrm_pol_hold(pol);
> @@ -832,8 +828,9 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
>  }
>  EXPORT_SYMBOL(xfrm_policy_bysel_ctx);
>  
> -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type,
> -				     int dir, u32 id, int delete, int *err)
> +struct xfrm_policy *
> +xfrm_policy_byid(struct net *net, const struct xfrm_mark *mark, u8 type,
> +		 int dir, u32 id, int delete, int *err)
>  {
>  	struct xfrm_policy *pol, *ret;
>  	struct hlist_head *chain;
> @@ -848,7 +845,7 @@ struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type,
>  	ret = NULL;
>  	hlist_for_each_entry(pol, chain, byidx) {
>  		if (pol->type == type && pol->index == id &&
> -		    (mark & pol->mark.m) == pol->mark.v) {
> +		    xfrm_policy_mark_match(mark, pol)) {
>  			xfrm_pol_hold(pol);
>  			if (delete) {
>  				*err = security_xfrm_policy_delete(
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 9018cb082e82..b84fe2a71080 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1820,7 +1820,6 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	struct km_event c;
>  	int delete;
>  	struct xfrm_mark m;
> -	u32 mark = xfrm_mark_get(attrs, &m);
>  
>  	p = nlmsg_data(nlh);
>  	delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY;
> @@ -1833,8 +1832,11 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (err)
>  		return err;
>  
> +	xfrm_mark_get(attrs, &m);
> +
>  	if (p->index)
> -		xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, delete, &err);
> +		xp = xfrm_policy_byid(net, &m, type, p->dir, p->index,
> +				      delete, &err);
>  	else {
>  		struct nlattr *rt = attrs[XFRMA_SEC_CTX];
>  		struct xfrm_sec_ctx *ctx;
> @@ -1851,7 +1853,7 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			if (err)
>  				return err;
>  		}
> -		xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, &p->sel,
> +		xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, &p->sel,
>  					   ctx, delete, &err);
>  		security_xfrm_policy_free(ctx);
>  	}
> @@ -2115,7 +2117,6 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	u8 type = XFRM_POLICY_TYPE_MAIN;
>  	int err = -ENOENT;
>  	struct xfrm_mark m;
> -	u32 mark = xfrm_mark_get(attrs, &m);
>  
>  	err = copy_from_user_policy_type(&type, attrs);
>  	if (err)
> @@ -2125,8 +2126,11 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (err)
>  		return err;
>  
> +	xfrm_mark_get(attrs, &m);
> +
>  	if (p->index)
> -		xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, 0, &err);
> +		xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, 0,
> +				      &err);
>  	else {
>  		struct nlattr *rt = attrs[XFRMA_SEC_CTX];
>  		struct xfrm_sec_ctx *ctx;
> @@ -2143,7 +2147,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			if (err)
>  				return err;
>  		}
> -		xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir,
> +		xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir,
>  					   &p->sel, ctx, 0, &err);
>  		security_xfrm_policy_free(ctx);
>  	}
> 

I've given this a careful eyeballing and it looks good to me, but I'd
also appreciate it if somebody gives this a good review as I'm quite
heat exhausted and hence may have made a mistake.

Acked-by: Colin Ian King <colin.king@canonical.com>
Marcelo Henrique Cerri Aug. 10, 2020, 5:58 p.m. UTC | #2
Acked-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>

On Mon, Aug 10, 2020 at 06:55:45PM +0200, Stefan Bader wrote:
> From: Xin Long <lucien.xin@gmail.com>
> 
> In commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"),
> it would take 'priority' to make a policy unique, and allow duplicated
> policies with different 'priority' to be added, which is not expected
> by userland, as Tobias reported in strongswan.
> 
> To fix this duplicated policies issue, and also fix the issue in
> commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"),
> when doing add/del/get/update on user interfaces, this patch is to change
> to look up a policy with both mark and mask by doing:
> 
>   mark.v == pol->mark.v && mark.m == pol->mark.m
> 
> and leave the check:
> 
>   (mark & pol->mark.m) == pol->mark.v
> 
> for tx/rx path only.
> 
> As the userland expects an exact mark and mask match to manage policies.
> 
> v1->v2:
>   - make xfrm_policy_mark_match inline and fix the changelog as
>     Tobias suggested.
> 
> Fixes: 295fae568885 ("xfrm: Allow user space manipulation of SPD mark")
> Fixes: ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list")
> Reported-by: Tobias Brunner <tobias@strongswan.org>
> Tested-by: Tobias Brunner <tobias@strongswan.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1890796
> 
> (backported from commit 4f47e8ab6ab796b5380f74866fa5287aca4dcc58)
> [smb: work around missing if_id parameter and __xfrm_policy_bysel_ctx]
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
> 
> This is my backport (it did compile ok on Bionic but if someone could
> double check this is looking sane) of the upstream change. The two
> functions primarily touched have one argument less there. It appeared to
> apply with some offset to Xenial as well but I have not compiled it
> there.
> 
> -Stefan
> 
> 
>  include/net/xfrm.h     |  8 +++++---
>  net/key/af_key.c       |  4 ++--
>  net/xfrm/xfrm_policy.c | 29 +++++++++++++----------------
>  net/xfrm/xfrm_user.c   | 16 ++++++++++------
>  4 files changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 83eb033f2cc6..b930c5c8e3d3 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -1674,13 +1674,15 @@ int xfrm_policy_walk(struct net *net, struct xfrm_policy_walk *walk,
>  		     void *);
>  void xfrm_policy_walk_done(struct xfrm_policy_walk *walk, struct net *net);
>  int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl);
> -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark,
> +struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net,
> +					  const struct xfrm_mark *mark,
>  					  u8 type, int dir,
>  					  struct xfrm_selector *sel,
>  					  struct xfrm_sec_ctx *ctx, int delete,
>  					  int *err);
> -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8, int dir,
> -				     u32 id, int delete, int *err);
> +struct xfrm_policy *xfrm_policy_byid(struct net *net,
> +				     const struct xfrm_mark *mark, u8,
> +				     int dir, u32 id, int delete, int *err);
>  int xfrm_policy_flush(struct net *net, u8 type, bool task_valid);
>  void xfrm_policy_hash_rebuild(struct net *net);
>  u32 xfrm_get_acqseq(void);
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index d0561c48234e..c8953fe2cfaa 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -2406,7 +2406,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa
>  			return err;
>  	}
>  
> -	xp = xfrm_policy_bysel_ctx(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN,
> +	xp = xfrm_policy_bysel_ctx(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN,
>  				   pol->sadb_x_policy_dir - 1, &sel, pol_ctx,
>  				   1, &err);
>  	security_xfrm_policy_free(pol_ctx);
> @@ -2657,7 +2657,7 @@ static int pfkey_spdget(struct sock *sk, struct sk_buff *skb, const struct sadb_
>  		return -EINVAL;
>  
>  	delete = (hdr->sadb_msg_type == SADB_X_SPDDELETE2);
> -	xp = xfrm_policy_byid(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN,
> +	xp = xfrm_policy_byid(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN,
>  			      dir, pol->sadb_x_policy_id, delete, &err);
>  	if (xp == NULL)
>  		return -ENOENT;
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 3dc1840c81b1..fb9a086395fe 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -718,14 +718,10 @@ static void xfrm_policy_requeue(struct xfrm_policy *old,
>  	spin_unlock_bh(&pq->hold_queue.lock);
>  }
>  
> -static bool xfrm_policy_mark_match(struct xfrm_policy *policy,
> -				   struct xfrm_policy *pol)
> +static inline bool xfrm_policy_mark_match(const struct xfrm_mark *mark,
> +					  struct xfrm_policy *pol)
>  {
> -	if (policy->mark.v == pol->mark.v &&
> -	    policy->priority == pol->priority)
> -		return true;
> -
> -	return false;
> +	return mark->v == pol->mark.v && mark->m == pol->mark.m;
>  }
>  
>  int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
> @@ -743,7 +739,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
>  	hlist_for_each_entry(pol, chain, bydst) {
>  		if (pol->type == policy->type &&
>  		    !selector_cmp(&pol->selector, &policy->selector) &&
> -		    xfrm_policy_mark_match(policy, pol) &&
> +		    xfrm_policy_mark_match(&policy->mark, pol) &&
>  		    xfrm_sec_ctx_match(pol->security, policy->security) &&
>  		    !WARN_ON(delpol)) {
>  			if (excl) {
> @@ -793,10 +789,10 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
>  }
>  EXPORT_SYMBOL(xfrm_policy_insert);
>  
> -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
> -					  int dir, struct xfrm_selector *sel,
> -					  struct xfrm_sec_ctx *ctx, int delete,
> -					  int *err)
> +struct xfrm_policy *
> +xfrm_policy_bysel_ctx(struct net *net, const struct xfrm_mark *mark, u8 type,
> +		      int dir, struct xfrm_selector *sel,
> +		      struct xfrm_sec_ctx *ctx, int delete, int *err)
>  {
>  	struct xfrm_policy *pol, *ret;
>  	struct hlist_head *chain;
> @@ -807,7 +803,7 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
>  	ret = NULL;
>  	hlist_for_each_entry(pol, chain, bydst) {
>  		if (pol->type == type &&
> -		    (mark & pol->mark.m) == pol->mark.v &&
> +		    xfrm_policy_mark_match(mark, pol) &&
>  		    !selector_cmp(sel, &pol->selector) &&
>  		    xfrm_sec_ctx_match(ctx, pol->security)) {
>  			xfrm_pol_hold(pol);
> @@ -832,8 +828,9 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
>  }
>  EXPORT_SYMBOL(xfrm_policy_bysel_ctx);
>  
> -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type,
> -				     int dir, u32 id, int delete, int *err)
> +struct xfrm_policy *
> +xfrm_policy_byid(struct net *net, const struct xfrm_mark *mark, u8 type,
> +		 int dir, u32 id, int delete, int *err)
>  {
>  	struct xfrm_policy *pol, *ret;
>  	struct hlist_head *chain;
> @@ -848,7 +845,7 @@ struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type,
>  	ret = NULL;
>  	hlist_for_each_entry(pol, chain, byidx) {
>  		if (pol->type == type && pol->index == id &&
> -		    (mark & pol->mark.m) == pol->mark.v) {
> +		    xfrm_policy_mark_match(mark, pol)) {
>  			xfrm_pol_hold(pol);
>  			if (delete) {
>  				*err = security_xfrm_policy_delete(
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 9018cb082e82..b84fe2a71080 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1820,7 +1820,6 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	struct km_event c;
>  	int delete;
>  	struct xfrm_mark m;
> -	u32 mark = xfrm_mark_get(attrs, &m);
>  
>  	p = nlmsg_data(nlh);
>  	delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY;
> @@ -1833,8 +1832,11 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (err)
>  		return err;
>  
> +	xfrm_mark_get(attrs, &m);
> +
>  	if (p->index)
> -		xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, delete, &err);
> +		xp = xfrm_policy_byid(net, &m, type, p->dir, p->index,
> +				      delete, &err);
>  	else {
>  		struct nlattr *rt = attrs[XFRMA_SEC_CTX];
>  		struct xfrm_sec_ctx *ctx;
> @@ -1851,7 +1853,7 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			if (err)
>  				return err;
>  		}
> -		xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, &p->sel,
> +		xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, &p->sel,
>  					   ctx, delete, &err);
>  		security_xfrm_policy_free(ctx);
>  	}
> @@ -2115,7 +2117,6 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	u8 type = XFRM_POLICY_TYPE_MAIN;
>  	int err = -ENOENT;
>  	struct xfrm_mark m;
> -	u32 mark = xfrm_mark_get(attrs, &m);
>  
>  	err = copy_from_user_policy_type(&type, attrs);
>  	if (err)
> @@ -2125,8 +2126,11 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (err)
>  		return err;
>  
> +	xfrm_mark_get(attrs, &m);
> +
>  	if (p->index)
> -		xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, 0, &err);
> +		xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, 0,
> +				      &err);
>  	else {
>  		struct nlattr *rt = attrs[XFRMA_SEC_CTX];
>  		struct xfrm_sec_ctx *ctx;
> @@ -2143,7 +2147,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			if (err)
>  				return err;
>  		}
> -		xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir,
> +		xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir,
>  					   &p->sel, ctx, 0, &err);
>  		security_xfrm_policy_free(ctx);
>  	}
> -- 
> 2.25.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Stefan Bader Aug. 11, 2020, 8:48 a.m. UTC | #3
On 10.08.20 18:55, Stefan Bader wrote:
> From: Xin Long <lucien.xin@gmail.com>
> 
> In commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"),
> it would take 'priority' to make a policy unique, and allow duplicated
> policies with different 'priority' to be added, which is not expected
> by userland, as Tobias reported in strongswan.
> 
> To fix this duplicated policies issue, and also fix the issue in
> commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"),
> when doing add/del/get/update on user interfaces, this patch is to change
> to look up a policy with both mark and mask by doing:
> 
>   mark.v == pol->mark.v && mark.m == pol->mark.m
> 
> and leave the check:
> 
>   (mark & pol->mark.m) == pol->mark.v
> 
> for tx/rx path only.
> 
> As the userland expects an exact mark and mask match to manage policies.
> 
> v1->v2:
>   - make xfrm_policy_mark_match inline and fix the changelog as
>     Tobias suggested.
> 
> Fixes: 295fae568885 ("xfrm: Allow user space manipulation of SPD mark")
> Fixes: ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list")
> Reported-by: Tobias Brunner <tobias@strongswan.org>
> Tested-by: Tobias Brunner <tobias@strongswan.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1890796
> 
> (backported from commit 4f47e8ab6ab796b5380f74866fa5287aca4dcc58)
> [smb: work around missing if_id parameter and __xfrm_policy_bysel_ctx]
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
> 
> This is my backport (it did compile ok on Bionic but if someone could
> double check this is looking sane) of the upstream change. The two
> functions primarily touched have one argument less there. It appeared to
> apply with some offset to Xenial as well but I have not compiled it
> there.

This looks to be applied by Kelsey to bionic,xenial/master-next. Thanks.

-Stefan

> 
> -Stefan
> 
> 
>  include/net/xfrm.h     |  8 +++++---
>  net/key/af_key.c       |  4 ++--
>  net/xfrm/xfrm_policy.c | 29 +++++++++++++----------------
>  net/xfrm/xfrm_user.c   | 16 ++++++++++------
>  4 files changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 83eb033f2cc6..b930c5c8e3d3 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -1674,13 +1674,15 @@ int xfrm_policy_walk(struct net *net, struct xfrm_policy_walk *walk,
>  		     void *);
>  void xfrm_policy_walk_done(struct xfrm_policy_walk *walk, struct net *net);
>  int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl);
> -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark,
> +struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net,
> +					  const struct xfrm_mark *mark,
>  					  u8 type, int dir,
>  					  struct xfrm_selector *sel,
>  					  struct xfrm_sec_ctx *ctx, int delete,
>  					  int *err);
> -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8, int dir,
> -				     u32 id, int delete, int *err);
> +struct xfrm_policy *xfrm_policy_byid(struct net *net,
> +				     const struct xfrm_mark *mark, u8,
> +				     int dir, u32 id, int delete, int *err);
>  int xfrm_policy_flush(struct net *net, u8 type, bool task_valid);
>  void xfrm_policy_hash_rebuild(struct net *net);
>  u32 xfrm_get_acqseq(void);
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index d0561c48234e..c8953fe2cfaa 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -2406,7 +2406,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa
>  			return err;
>  	}
>  
> -	xp = xfrm_policy_bysel_ctx(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN,
> +	xp = xfrm_policy_bysel_ctx(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN,
>  				   pol->sadb_x_policy_dir - 1, &sel, pol_ctx,
>  				   1, &err);
>  	security_xfrm_policy_free(pol_ctx);
> @@ -2657,7 +2657,7 @@ static int pfkey_spdget(struct sock *sk, struct sk_buff *skb, const struct sadb_
>  		return -EINVAL;
>  
>  	delete = (hdr->sadb_msg_type == SADB_X_SPDDELETE2);
> -	xp = xfrm_policy_byid(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN,
> +	xp = xfrm_policy_byid(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN,
>  			      dir, pol->sadb_x_policy_id, delete, &err);
>  	if (xp == NULL)
>  		return -ENOENT;
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 3dc1840c81b1..fb9a086395fe 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -718,14 +718,10 @@ static void xfrm_policy_requeue(struct xfrm_policy *old,
>  	spin_unlock_bh(&pq->hold_queue.lock);
>  }
>  
> -static bool xfrm_policy_mark_match(struct xfrm_policy *policy,
> -				   struct xfrm_policy *pol)
> +static inline bool xfrm_policy_mark_match(const struct xfrm_mark *mark,
> +					  struct xfrm_policy *pol)
>  {
> -	if (policy->mark.v == pol->mark.v &&
> -	    policy->priority == pol->priority)
> -		return true;
> -
> -	return false;
> +	return mark->v == pol->mark.v && mark->m == pol->mark.m;
>  }
>  
>  int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
> @@ -743,7 +739,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
>  	hlist_for_each_entry(pol, chain, bydst) {
>  		if (pol->type == policy->type &&
>  		    !selector_cmp(&pol->selector, &policy->selector) &&
> -		    xfrm_policy_mark_match(policy, pol) &&
> +		    xfrm_policy_mark_match(&policy->mark, pol) &&
>  		    xfrm_sec_ctx_match(pol->security, policy->security) &&
>  		    !WARN_ON(delpol)) {
>  			if (excl) {
> @@ -793,10 +789,10 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
>  }
>  EXPORT_SYMBOL(xfrm_policy_insert);
>  
> -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
> -					  int dir, struct xfrm_selector *sel,
> -					  struct xfrm_sec_ctx *ctx, int delete,
> -					  int *err)
> +struct xfrm_policy *
> +xfrm_policy_bysel_ctx(struct net *net, const struct xfrm_mark *mark, u8 type,
> +		      int dir, struct xfrm_selector *sel,
> +		      struct xfrm_sec_ctx *ctx, int delete, int *err)
>  {
>  	struct xfrm_policy *pol, *ret;
>  	struct hlist_head *chain;
> @@ -807,7 +803,7 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
>  	ret = NULL;
>  	hlist_for_each_entry(pol, chain, bydst) {
>  		if (pol->type == type &&
> -		    (mark & pol->mark.m) == pol->mark.v &&
> +		    xfrm_policy_mark_match(mark, pol) &&
>  		    !selector_cmp(sel, &pol->selector) &&
>  		    xfrm_sec_ctx_match(ctx, pol->security)) {
>  			xfrm_pol_hold(pol);
> @@ -832,8 +828,9 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
>  }
>  EXPORT_SYMBOL(xfrm_policy_bysel_ctx);
>  
> -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type,
> -				     int dir, u32 id, int delete, int *err)
> +struct xfrm_policy *
> +xfrm_policy_byid(struct net *net, const struct xfrm_mark *mark, u8 type,
> +		 int dir, u32 id, int delete, int *err)
>  {
>  	struct xfrm_policy *pol, *ret;
>  	struct hlist_head *chain;
> @@ -848,7 +845,7 @@ struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type,
>  	ret = NULL;
>  	hlist_for_each_entry(pol, chain, byidx) {
>  		if (pol->type == type && pol->index == id &&
> -		    (mark & pol->mark.m) == pol->mark.v) {
> +		    xfrm_policy_mark_match(mark, pol)) {
>  			xfrm_pol_hold(pol);
>  			if (delete) {
>  				*err = security_xfrm_policy_delete(
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 9018cb082e82..b84fe2a71080 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1820,7 +1820,6 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	struct km_event c;
>  	int delete;
>  	struct xfrm_mark m;
> -	u32 mark = xfrm_mark_get(attrs, &m);
>  
>  	p = nlmsg_data(nlh);
>  	delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY;
> @@ -1833,8 +1832,11 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (err)
>  		return err;
>  
> +	xfrm_mark_get(attrs, &m);
> +
>  	if (p->index)
> -		xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, delete, &err);
> +		xp = xfrm_policy_byid(net, &m, type, p->dir, p->index,
> +				      delete, &err);
>  	else {
>  		struct nlattr *rt = attrs[XFRMA_SEC_CTX];
>  		struct xfrm_sec_ctx *ctx;
> @@ -1851,7 +1853,7 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			if (err)
>  				return err;
>  		}
> -		xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, &p->sel,
> +		xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, &p->sel,
>  					   ctx, delete, &err);
>  		security_xfrm_policy_free(ctx);
>  	}
> @@ -2115,7 +2117,6 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	u8 type = XFRM_POLICY_TYPE_MAIN;
>  	int err = -ENOENT;
>  	struct xfrm_mark m;
> -	u32 mark = xfrm_mark_get(attrs, &m);
>  
>  	err = copy_from_user_policy_type(&type, attrs);
>  	if (err)
> @@ -2125,8 +2126,11 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (err)
>  		return err;
>  
> +	xfrm_mark_get(attrs, &m);
> +
>  	if (p->index)
> -		xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, 0, &err);
> +		xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, 0,
> +				      &err);
>  	else {
>  		struct nlattr *rt = attrs[XFRMA_SEC_CTX];
>  		struct xfrm_sec_ctx *ctx;
> @@ -2143,7 +2147,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			if (err)
>  				return err;
>  		}
> -		xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir,
> +		xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir,
>  					   &p->sel, ctx, 0, &err);
>  		security_xfrm_policy_free(ctx);
>  	}
>
diff mbox series

Patch

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 83eb033f2cc6..b930c5c8e3d3 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1674,13 +1674,15 @@  int xfrm_policy_walk(struct net *net, struct xfrm_policy_walk *walk,
 		     void *);
 void xfrm_policy_walk_done(struct xfrm_policy_walk *walk, struct net *net);
 int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl);
-struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark,
+struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net,
+					  const struct xfrm_mark *mark,
 					  u8 type, int dir,
 					  struct xfrm_selector *sel,
 					  struct xfrm_sec_ctx *ctx, int delete,
 					  int *err);
-struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8, int dir,
-				     u32 id, int delete, int *err);
+struct xfrm_policy *xfrm_policy_byid(struct net *net,
+				     const struct xfrm_mark *mark, u8,
+				     int dir, u32 id, int delete, int *err);
 int xfrm_policy_flush(struct net *net, u8 type, bool task_valid);
 void xfrm_policy_hash_rebuild(struct net *net);
 u32 xfrm_get_acqseq(void);
diff --git a/net/key/af_key.c b/net/key/af_key.c
index d0561c48234e..c8953fe2cfaa 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -2406,7 +2406,7 @@  static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa
 			return err;
 	}
 
-	xp = xfrm_policy_bysel_ctx(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN,
+	xp = xfrm_policy_bysel_ctx(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN,
 				   pol->sadb_x_policy_dir - 1, &sel, pol_ctx,
 				   1, &err);
 	security_xfrm_policy_free(pol_ctx);
@@ -2657,7 +2657,7 @@  static int pfkey_spdget(struct sock *sk, struct sk_buff *skb, const struct sadb_
 		return -EINVAL;
 
 	delete = (hdr->sadb_msg_type == SADB_X_SPDDELETE2);
-	xp = xfrm_policy_byid(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN,
+	xp = xfrm_policy_byid(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN,
 			      dir, pol->sadb_x_policy_id, delete, &err);
 	if (xp == NULL)
 		return -ENOENT;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 3dc1840c81b1..fb9a086395fe 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -718,14 +718,10 @@  static void xfrm_policy_requeue(struct xfrm_policy *old,
 	spin_unlock_bh(&pq->hold_queue.lock);
 }
 
-static bool xfrm_policy_mark_match(struct xfrm_policy *policy,
-				   struct xfrm_policy *pol)
+static inline bool xfrm_policy_mark_match(const struct xfrm_mark *mark,
+					  struct xfrm_policy *pol)
 {
-	if (policy->mark.v == pol->mark.v &&
-	    policy->priority == pol->priority)
-		return true;
-
-	return false;
+	return mark->v == pol->mark.v && mark->m == pol->mark.m;
 }
 
 int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
@@ -743,7 +739,7 @@  int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
 	hlist_for_each_entry(pol, chain, bydst) {
 		if (pol->type == policy->type &&
 		    !selector_cmp(&pol->selector, &policy->selector) &&
-		    xfrm_policy_mark_match(policy, pol) &&
+		    xfrm_policy_mark_match(&policy->mark, pol) &&
 		    xfrm_sec_ctx_match(pol->security, policy->security) &&
 		    !WARN_ON(delpol)) {
 			if (excl) {
@@ -793,10 +789,10 @@  int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
 }
 EXPORT_SYMBOL(xfrm_policy_insert);
 
-struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
-					  int dir, struct xfrm_selector *sel,
-					  struct xfrm_sec_ctx *ctx, int delete,
-					  int *err)
+struct xfrm_policy *
+xfrm_policy_bysel_ctx(struct net *net, const struct xfrm_mark *mark, u8 type,
+		      int dir, struct xfrm_selector *sel,
+		      struct xfrm_sec_ctx *ctx, int delete, int *err)
 {
 	struct xfrm_policy *pol, *ret;
 	struct hlist_head *chain;
@@ -807,7 +803,7 @@  struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
 	ret = NULL;
 	hlist_for_each_entry(pol, chain, bydst) {
 		if (pol->type == type &&
-		    (mark & pol->mark.m) == pol->mark.v &&
+		    xfrm_policy_mark_match(mark, pol) &&
 		    !selector_cmp(sel, &pol->selector) &&
 		    xfrm_sec_ctx_match(ctx, pol->security)) {
 			xfrm_pol_hold(pol);
@@ -832,8 +828,9 @@  struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type,
 }
 EXPORT_SYMBOL(xfrm_policy_bysel_ctx);
 
-struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type,
-				     int dir, u32 id, int delete, int *err)
+struct xfrm_policy *
+xfrm_policy_byid(struct net *net, const struct xfrm_mark *mark, u8 type,
+		 int dir, u32 id, int delete, int *err)
 {
 	struct xfrm_policy *pol, *ret;
 	struct hlist_head *chain;
@@ -848,7 +845,7 @@  struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type,
 	ret = NULL;
 	hlist_for_each_entry(pol, chain, byidx) {
 		if (pol->type == type && pol->index == id &&
-		    (mark & pol->mark.m) == pol->mark.v) {
+		    xfrm_policy_mark_match(mark, pol)) {
 			xfrm_pol_hold(pol);
 			if (delete) {
 				*err = security_xfrm_policy_delete(
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 9018cb082e82..b84fe2a71080 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1820,7 +1820,6 @@  static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct km_event c;
 	int delete;
 	struct xfrm_mark m;
-	u32 mark = xfrm_mark_get(attrs, &m);
 
 	p = nlmsg_data(nlh);
 	delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY;
@@ -1833,8 +1832,11 @@  static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err)
 		return err;
 
+	xfrm_mark_get(attrs, &m);
+
 	if (p->index)
-		xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, delete, &err);
+		xp = xfrm_policy_byid(net, &m, type, p->dir, p->index,
+				      delete, &err);
 	else {
 		struct nlattr *rt = attrs[XFRMA_SEC_CTX];
 		struct xfrm_sec_ctx *ctx;
@@ -1851,7 +1853,7 @@  static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 			if (err)
 				return err;
 		}
-		xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, &p->sel,
+		xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, &p->sel,
 					   ctx, delete, &err);
 		security_xfrm_policy_free(ctx);
 	}
@@ -2115,7 +2117,6 @@  static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 	u8 type = XFRM_POLICY_TYPE_MAIN;
 	int err = -ENOENT;
 	struct xfrm_mark m;
-	u32 mark = xfrm_mark_get(attrs, &m);
 
 	err = copy_from_user_policy_type(&type, attrs);
 	if (err)
@@ -2125,8 +2126,11 @@  static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err)
 		return err;
 
+	xfrm_mark_get(attrs, &m);
+
 	if (p->index)
-		xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, 0, &err);
+		xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, 0,
+				      &err);
 	else {
 		struct nlattr *rt = attrs[XFRMA_SEC_CTX];
 		struct xfrm_sec_ctx *ctx;
@@ -2143,7 +2147,7 @@  static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 			if (err)
 				return err;
 		}
-		xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir,
+		xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir,
 					   &p->sel, ctx, 0, &err);
 		security_xfrm_policy_free(ctx);
 	}