* [PATCH net-next] xfrm: policy: Remove obsolete WARN while xfrm policy inserting @ 2020-03-27 12:34 YueHaibing 2020-03-28 11:23 ` Steffen Klassert 0 siblings, 1 reply; 8+ messages in thread From: YueHaibing @ 2020-03-27 12:34 UTC (permalink / raw) To: steffen.klassert, herbert, davem, kuba; +Cc: netdev, linux-kernel, YueHaibing Since commit 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities"), we allow duplicate policies with different priority, this WARN is not needed any more. Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- net/xfrm/xfrm_policy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index dbda08e..5c4387c 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -1508,7 +1508,7 @@ static void xfrm_policy_insert_inexact_list(struct hlist_head *chain, !selector_cmp(&pol->selector, &policy->selector) && xfrm_policy_mark_match(policy, pol) && xfrm_sec_ctx_match(pol->security, policy->security) && - !WARN_ON(delpol)) { + !delpol) { delpol = pol; if (policy->priority > pol->priority) continue; @@ -1543,7 +1543,7 @@ static struct xfrm_policy *xfrm_policy_insert_list(struct hlist_head *chain, !selector_cmp(&pol->selector, &policy->selector) && xfrm_policy_mark_match(policy, pol) && xfrm_sec_ctx_match(pol->security, policy->security) && - !WARN_ON(delpol)) { + !delpol) { if (excl) return ERR_PTR(-EEXIST); delpol = pol; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] xfrm: policy: Remove obsolete WARN while xfrm policy inserting 2020-03-27 12:34 [PATCH net-next] xfrm: policy: Remove obsolete WARN while xfrm policy inserting YueHaibing @ 2020-03-28 11:23 ` Steffen Klassert 2020-03-30 14:05 ` Yuehaibing 0 siblings, 1 reply; 8+ messages in thread From: Steffen Klassert @ 2020-03-28 11:23 UTC (permalink / raw) To: YueHaibing; +Cc: herbert, davem, kuba, netdev, linux-kernel On Fri, Mar 27, 2020 at 08:34:43PM +0800, YueHaibing wrote: > Since commit 7cb8a93968e3 ("xfrm: Allow inserting policies with matching > mark and different priorities"), we allow duplicate policies with > different priority, this WARN is not needed any more. Can you please describe a bit more detailed why this warning can't trigger anymore? Thanks! ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] xfrm: policy: Remove obsolete WARN while xfrm policy inserting 2020-03-28 11:23 ` Steffen Klassert @ 2020-03-30 14:05 ` Yuehaibing 2020-04-06 9:03 ` Steffen Klassert 0 siblings, 1 reply; 8+ messages in thread From: Yuehaibing @ 2020-03-30 14:05 UTC (permalink / raw) To: Steffen Klassert; +Cc: herbert, davem, kuba, netdev, linux-kernel On 2020/3/28 19:23, Steffen Klassert wrote: > On Fri, Mar 27, 2020 at 08:34:43PM +0800, YueHaibing wrote: >> Since commit 7cb8a93968e3 ("xfrm: Allow inserting policies with matching >> mark and different priorities"), we allow duplicate policies with >> different priority, this WARN is not needed any more. > > Can you please describe a bit more detailed why this warning > can't trigger anymore? No, this warning is triggered while detect a duplicate entry in the policy list regardless of the priority. If we insert policy like this: policy A (mark.v = 3475289, mark.m = 0, priority = 1) //A is inserted policy B (mark.v = 0, mark.m = 0, priority = 0) //B is inserted policy C (mark.v = 3475289, mark.m = 0, priority = 0) //C is inserted and B is deleted policy D (mark.v = 3475289, mark.m = 0, priority = 1) while finding delpol in xfrm_policy_insert_list, first round delpol is matched C, whose priority is less than D, so contiue the loop, then A is matched, WARN_ON is triggered. It seems the WARN is useless. > > Thanks! > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] xfrm: policy: Remove obsolete WARN while xfrm policy inserting 2020-03-30 14:05 ` Yuehaibing @ 2020-04-06 9:03 ` Steffen Klassert 2020-04-09 8:19 ` Yuehaibing 0 siblings, 1 reply; 8+ messages in thread From: Steffen Klassert @ 2020-04-06 9:03 UTC (permalink / raw) To: Yuehaibing; +Cc: herbert, davem, kuba, netdev, linux-kernel On Mon, Mar 30, 2020 at 10:05:32PM +0800, Yuehaibing wrote: > On 2020/3/28 19:23, Steffen Klassert wrote: > > On Fri, Mar 27, 2020 at 08:34:43PM +0800, YueHaibing wrote: > >> Since commit 7cb8a93968e3 ("xfrm: Allow inserting policies with matching > >> mark and different priorities"), we allow duplicate policies with > >> different priority, this WARN is not needed any more. > > > > Can you please describe a bit more detailed why this warning > > can't trigger anymore? > > No, this warning is triggered while detect a duplicate entry in the policy list > > regardless of the priority. If we insert policy like this: > > policy A (mark.v = 3475289, mark.m = 0, priority = 1) //A is inserted > policy B (mark.v = 0, mark.m = 0, priority = 0) //B is inserted > policy C (mark.v = 3475289, mark.m = 0, priority = 0) //C is inserted and B is deleted The codepath that replaces a policy by another should just trigger on policy updates (XFRM_MSG_UPDPOLICY). Is that the case in your test? It should not be possible to add policy C with XFRM_MSG_NEWPOLICY as long as you have policy B inserted. The update replaces an old policy by a new one, the lookup keys of the old policy must match the lookup keys of the new one. But policy B has not the same lookup keys as C, the mark is different. So B should not be replaced with C. > policy D (mark.v = 3475289, mark.m = 0, priority = 1) > > while finding delpol in xfrm_policy_insert_list, > first round delpol is matched C, whose priority is less than D, so contiue the loop, > then A is matched, WARN_ON is triggered. It seems the WARN is useless. Looks like the warning is usefull, it found a bug. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] xfrm: policy: Remove obsolete WARN while xfrm policy inserting 2020-04-06 9:03 ` Steffen Klassert @ 2020-04-09 8:19 ` Yuehaibing 2020-04-15 7:14 ` Steffen Klassert 0 siblings, 1 reply; 8+ messages in thread From: Yuehaibing @ 2020-04-09 8:19 UTC (permalink / raw) To: Steffen Klassert; +Cc: herbert, davem, kuba, netdev, linux-kernel On 2020/4/6 17:03, Steffen Klassert wrote: > On Mon, Mar 30, 2020 at 10:05:32PM +0800, Yuehaibing wrote: >> On 2020/3/28 19:23, Steffen Klassert wrote: >>> On Fri, Mar 27, 2020 at 08:34:43PM +0800, YueHaibing wrote: >>>> Since commit 7cb8a93968e3 ("xfrm: Allow inserting policies with matching >>>> mark and different priorities"), we allow duplicate policies with >>>> different priority, this WARN is not needed any more. >>> >>> Can you please describe a bit more detailed why this warning >>> can't trigger anymore? >> >> No, this warning is triggered while detect a duplicate entry in the policy list >> >> regardless of the priority. If we insert policy like this: >> >> policy A (mark.v = 3475289, mark.m = 0, priority = 1) //A is inserted >> policy B (mark.v = 0, mark.m = 0, priority = 0) //B is inserted >> policy C (mark.v = 3475289, mark.m = 0, priority = 0) //C is inserted and B is deleted > > The codepath that replaces a policy by another should just trigger > on policy updates (XFRM_MSG_UPDPOLICY). Is that the case in your > test? Yes, this is triggered by XFRM_MSG_UPDPOLICY > > It should not be possible to add policy C with XFRM_MSG_NEWPOLICY > as long as you have policy B inserted. > > The update replaces an old policy by a new one, the lookup keys of > the old policy must match the lookup keys of the new one. But policy > B has not the same lookup keys as C, the mark is different. So B should > not be replaced with C. 1436 static bool xfrm_policy_mark_match(struct xfrm_policy *policy, 1437 struct xfrm_policy *pol) 1438 { 1439 u32 mark = policy->mark.v & policy->mark.m; 1440 1441 if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) 1442 return true; 1443 1444 if ((mark & pol->mark.m) == pol->mark.v && //policy is C, pol is B, so mark is 0, pol->mark.m is 0, pol->mark.v is 0 1445 policy->priority == pol->priority) //priority is same zero, so return true, B is replaced with C 1446 return true; 1447 1448 return false; 1449 } Should xfrm_policy_mark_match be fixed? > >> policy D (mark.v = 3475289, mark.m = 0, priority = 1) >> >> while finding delpol in xfrm_policy_insert_list, >> first round delpol is matched C, whose priority is less than D, so contiue the loop, >> then A is matched, WARN_ON is triggered. It seems the WARN is useless. > > Looks like the warning is usefull, it found a bug. > > > . > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] xfrm: policy: Remove obsolete WARN while xfrm policy inserting 2020-04-09 8:19 ` Yuehaibing @ 2020-04-15 7:14 ` Steffen Klassert 2020-04-17 11:01 ` Yuehaibing 0 siblings, 1 reply; 8+ messages in thread From: Steffen Klassert @ 2020-04-15 7:14 UTC (permalink / raw) To: Yuehaibing; +Cc: herbert, davem, kuba, netdev, linux-kernel On Thu, Apr 09, 2020 at 04:19:37PM +0800, Yuehaibing wrote: > > > On 2020/4/6 17:03, Steffen Klassert wrote: > > On Mon, Mar 30, 2020 at 10:05:32PM +0800, Yuehaibing wrote: > >> On 2020/3/28 19:23, Steffen Klassert wrote: > >>> On Fri, Mar 27, 2020 at 08:34:43PM +0800, YueHaibing wrote: > >>>> Since commit 7cb8a93968e3 ("xfrm: Allow inserting policies with matching > >>>> mark and different priorities"), we allow duplicate policies with > >>>> different priority, this WARN is not needed any more. > >>> > >>> Can you please describe a bit more detailed why this warning > >>> can't trigger anymore? > >> > >> No, this warning is triggered while detect a duplicate entry in the policy list > >> > >> regardless of the priority. If we insert policy like this: > >> > >> policy A (mark.v = 3475289, mark.m = 0, priority = 1) //A is inserted > >> policy B (mark.v = 0, mark.m = 0, priority = 0) //B is inserted > >> policy C (mark.v = 3475289, mark.m = 0, priority = 0) //C is inserted and B is deleted > > > > The codepath that replaces a policy by another should just trigger > > on policy updates (XFRM_MSG_UPDPOLICY). Is that the case in your > > test? > > Yes, this is triggered by XFRM_MSG_UPDPOLICY > > > > > It should not be possible to add policy C with XFRM_MSG_NEWPOLICY > > as long as you have policy B inserted. > > > > The update replaces an old policy by a new one, the lookup keys of > > the old policy must match the lookup keys of the new one. But policy > > B has not the same lookup keys as C, the mark is different. So B should > > not be replaced with C. > > 1436 static bool xfrm_policy_mark_match(struct xfrm_policy *policy, > 1437 struct xfrm_policy *pol) > 1438 { > 1439 u32 mark = policy->mark.v & policy->mark.m; > 1440 > 1441 if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) > 1442 return true; > 1443 > 1444 if ((mark & pol->mark.m) == pol->mark.v && //policy is C, pol is B, so mark is 0, pol->mark.m is 0, pol->mark.v is 0 > 1445 policy->priority == pol->priority) //priority is same zero, so return true, B is replaced with C > 1446 return true; > 1447 > 1448 return false; > 1449 } > > Should xfrm_policy_mark_match be fixed? Yes, xfrm_policy_mark_match should only replace if the found policy has the same lookup keys. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] xfrm: policy: Remove obsolete WARN while xfrm policy inserting 2020-04-15 7:14 ` Steffen Klassert @ 2020-04-17 11:01 ` Yuehaibing 2020-04-21 6:28 ` Steffen Klassert 0 siblings, 1 reply; 8+ messages in thread From: Yuehaibing @ 2020-04-17 11:01 UTC (permalink / raw) To: Steffen Klassert; +Cc: herbert, davem, kuba, netdev, linux-kernel On 2020/4/15 15:14, Steffen Klassert wrote: > On Thu, Apr 09, 2020 at 04:19:37PM +0800, Yuehaibing wrote: >> >> >> On 2020/4/6 17:03, Steffen Klassert wrote: >>> On Mon, Mar 30, 2020 at 10:05:32PM +0800, Yuehaibing wrote: >>>> On 2020/3/28 19:23, Steffen Klassert wrote: >>>>> On Fri, Mar 27, 2020 at 08:34:43PM +0800, YueHaibing wrote: >>>>>> Since commit 7cb8a93968e3 ("xfrm: Allow inserting policies with matching >>>>>> mark and different priorities"), we allow duplicate policies with >>>>>> different priority, this WARN is not needed any more. >>>>> >>>>> Can you please describe a bit more detailed why this warning >>>>> can't trigger anymore? >>>> >>>> No, this warning is triggered while detect a duplicate entry in the policy list >>>> >>>> regardless of the priority. If we insert policy like this: >>>> >>>> policy A (mark.v = 3475289, mark.m = 0, priority = 1) //A is inserted >>>> policy B (mark.v = 0, mark.m = 0, priority = 0) //B is inserted >>>> policy C (mark.v = 3475289, mark.m = 0, priority = 0) //C is inserted and B is deleted >>> >>> The codepath that replaces a policy by another should just trigger >>> on policy updates (XFRM_MSG_UPDPOLICY). Is that the case in your >>> test? >> >> Yes, this is triggered by XFRM_MSG_UPDPOLICY >> >>> >>> It should not be possible to add policy C with XFRM_MSG_NEWPOLICY >>> as long as you have policy B inserted. >>> >>> The update replaces an old policy by a new one, the lookup keys of >>> the old policy must match the lookup keys of the new one. But policy >>> B has not the same lookup keys as C, the mark is different. So B should >>> not be replaced with C. >> >> 1436 static bool xfrm_policy_mark_match(struct xfrm_policy *policy, >> 1437 struct xfrm_policy *pol) >> 1438 { >> 1439 u32 mark = policy->mark.v & policy->mark.m; >> 1440 >> 1441 if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) >> 1442 return true; >> 1443 >> 1444 if ((mark & pol->mark.m) == pol->mark.v && //policy is C, pol is B, so mark is 0, pol->mark.m is 0, pol->mark.v is 0 >> 1445 policy->priority == pol->priority) //priority is same zero, so return true, B is replaced with C >> 1446 return true; >> 1447 >> 1448 return false; >> 1449 } >> >> Should xfrm_policy_mark_match be fixed? > > Yes, xfrm_policy_mark_match should only replace if the found > policy has the same lookup keys. I'm wonder that lookup keys means association of mark.v and mark.m, or the mark (mark.v & mark.m). In above my case, policy B and C has the same mark (that is 0), if the lookup keys is mark, replacement is permitted. If lookup keys is association of mark.v and mark.m, then: policy E (mark.v = 0x1, mark.m = 0x3, priority = 1) policy F (mark.v = 0x1, mark.m = 0x5, priority = 1) E should not be replaced by F, but this is permitted now. > > . > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] xfrm: policy: Remove obsolete WARN while xfrm policy inserting 2020-04-17 11:01 ` Yuehaibing @ 2020-04-21 6:28 ` Steffen Klassert 0 siblings, 0 replies; 8+ messages in thread From: Steffen Klassert @ 2020-04-21 6:28 UTC (permalink / raw) To: Yuehaibing; +Cc: herbert, davem, kuba, netdev, linux-kernel On Fri, Apr 17, 2020 at 07:01:52PM +0800, Yuehaibing wrote: > On 2020/4/15 15:14, Steffen Klassert wrote: > > On Thu, Apr 09, 2020 at 04:19:37PM +0800, Yuehaibing wrote: > >> > >> > >> On 2020/4/6 17:03, Steffen Klassert wrote: > >>> On Mon, Mar 30, 2020 at 10:05:32PM +0800, Yuehaibing wrote: > >>>> On 2020/3/28 19:23, Steffen Klassert wrote: > >>>>> On Fri, Mar 27, 2020 at 08:34:43PM +0800, YueHaibing wrote: > >>>>>> Since commit 7cb8a93968e3 ("xfrm: Allow inserting policies with matching > >>>>>> mark and different priorities"), we allow duplicate policies with > >>>>>> different priority, this WARN is not needed any more. > >>>>> > >>>>> Can you please describe a bit more detailed why this warning > >>>>> can't trigger anymore? > >>>> > >>>> No, this warning is triggered while detect a duplicate entry in the policy list > >>>> > >>>> regardless of the priority. If we insert policy like this: > >>>> > >>>> policy A (mark.v = 3475289, mark.m = 0, priority = 1) //A is inserted > >>>> policy B (mark.v = 0, mark.m = 0, priority = 0) //B is inserted > >>>> policy C (mark.v = 3475289, mark.m = 0, priority = 0) //C is inserted and B is deleted > >>> > >>> The codepath that replaces a policy by another should just trigger > >>> on policy updates (XFRM_MSG_UPDPOLICY). Is that the case in your > >>> test? > >> > >> Yes, this is triggered by XFRM_MSG_UPDPOLICY > >> > >>> > >>> It should not be possible to add policy C with XFRM_MSG_NEWPOLICY > >>> as long as you have policy B inserted. > >>> > >>> The update replaces an old policy by a new one, the lookup keys of > >>> the old policy must match the lookup keys of the new one. But policy > >>> B has not the same lookup keys as C, the mark is different. So B should > >>> not be replaced with C. > >> > >> 1436 static bool xfrm_policy_mark_match(struct xfrm_policy *policy, > >> 1437 struct xfrm_policy *pol) > >> 1438 { > >> 1439 u32 mark = policy->mark.v & policy->mark.m; > >> 1440 > >> 1441 if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) > >> 1442 return true; > >> 1443 > >> 1444 if ((mark & pol->mark.m) == pol->mark.v && //policy is C, pol is B, so mark is 0, pol->mark.m is 0, pol->mark.v is 0 > >> 1445 policy->priority == pol->priority) //priority is same zero, so return true, B is replaced with C > >> 1446 return true; > >> 1447 > >> 1448 return false; > >> 1449 } > >> > >> Should xfrm_policy_mark_match be fixed? > > > > Yes, xfrm_policy_mark_match should only replace if the found > > policy has the same lookup keys. > > I'm wonder that lookup keys means association of mark.v and mark.m, or the mark (mark.v & mark.m). Good point. I'd say the lookup lookup keys are identical if the policy lookup can't distinguish between the policies. So (mark.v & mark.m) should be it. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-04-21 6:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-27 12:34 [PATCH net-next] xfrm: policy: Remove obsolete WARN while xfrm policy inserting YueHaibing 2020-03-28 11:23 ` Steffen Klassert 2020-03-30 14:05 ` Yuehaibing 2020-04-06 9:03 ` Steffen Klassert 2020-04-09 8:19 ` Yuehaibing 2020-04-15 7:14 ` Steffen Klassert 2020-04-17 11:01 ` Yuehaibing 2020-04-21 6:28 ` Steffen Klassert
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).