From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BF8F8C433DF for ; Mon, 25 May 2020 03:05:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id AA39A207C3 for ; Mon, 25 May 2020 03:05:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388703AbgEYDFJ (ORCPT ); Sun, 24 May 2020 23:05:09 -0400 Received: from szxga04-in.huawei.com ([45.249.212.190]:4893 "EHLO huawei.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S2388178AbgEYDFJ (ORCPT ); Sun, 24 May 2020 23:05:09 -0400 Received: from DGGEMS402-HUB.china.huawei.com (unknown [172.30.72.60]) by Forcepoint Email with ESMTP id AA5478C8C87755055AB5; Mon, 25 May 2020 11:05:03 +0800 (CST) Received: from [127.0.0.1] (10.166.215.154) by DGGEMS402-HUB.china.huawei.com (10.3.19.202) with Microsoft SMTP Server id 14.3.487.0; Mon, 25 May 2020 11:04:59 +0800 Subject: Re: [PATCH v2] xfrm: policy: Fix xfrm policy match To: Xin Long References: <20200421143149.45108-1-yuehaibing@huawei.com> <20200422125346.27756-1-yuehaibing@huawei.com> <0015ec4c-0e9c-a9d2-eb03-4d51c5fbbe86@huawei.com> <20200519085353.GE13121@gauss3.secunet.de> <550a82f1-9cb3-2392-25c6-b2a84a00ca33@huawei.com> <1c4c5d40-1e35-f9bb-3f17-01bb4675f3aa@huawei.com> CC: Steffen Klassert , Herbert Xu , davem , Jakub Kicinski , network dev , LKML From: Yuehaibing Message-ID: Date: Mon, 25 May 2020 11:04:58 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.166.215.154] X-CFilter-Loop: Reflected Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 2020/5/23 17:02, Xin Long wrote: > On Fri, May 22, 2020 at 8:39 PM Yuehaibing wrote: >> >> On 2020/5/22 13:49, Xin Long wrote: >>> On Fri, May 22, 2020 at 9:45 AM Yuehaibing wrote: >>>> >>>> On 2020/5/21 14:49, Xin Long wrote: >>>>> On Tue, May 19, 2020 at 4:53 PM Steffen Klassert >>>>> wrote: >>>>>> >>>>>> On Fri, May 15, 2020 at 04:39:57PM +0800, Yuehaibing wrote: >>>>>>> >>>>>>> Friendly ping... >>>>>>> >>>>>>> Any plan for this issue? >>>>>> >>>>>> There was still no consensus between you and Xin on how >>>>>> to fix this issue. Once this happens, I consider applying >>>>>> a fix. >>>>>> >>>>> Sorry, Yuehaibing, I can't really accept to do: (A->mark.m & A->mark.v) >>>>> I'm thinking to change to: >>>>> >>>>> static bool xfrm_policy_mark_match(struct xfrm_policy *policy, >>>>> struct xfrm_policy *pol) >>>>> { >>>>> - u32 mark = policy->mark.v & policy->mark.m; >>>>> - >>>>> - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) >>>>> - return true; >>>>> - >>>>> - if ((mark & pol->mark.m) == pol->mark.v && >>>>> - policy->priority == pol->priority) >>>>> + if (policy->mark.v == pol->mark.v && >>>>> + (policy->mark.m == pol->mark.m || >>>>> + policy->priority == pol->priority)) >>>>> return true; >>>>> >>>>> return false; >>>>> >>>>> which means we consider (the same value and mask) or >>>>> (the same value and priority) as the same one. This will >>>>> cover both problems. >>>> >>>> policy A (mark.v = 0x1011, mark.m = 0x1011, priority = 1) >>>> policy B (mark.v = 0x1001, mark.m = 0x1001, priority = 1) >>> I'd think these are 2 different policies. >>> >>>> >>>> when fl->flowi_mark == 0x12341011, in xfrm_policy_match() do check like this: >>>> >>>> (fl->flowi_mark & pol->mark.m) != pol->mark.v >>>> >>>> 0x12341011 & 0x1011 == 0x00001011 >>>> 0x12341011 & 0x1001 == 0x00001001 >>>> >>>> This also match different policy depends on the order of policy inserting. >>> Yes, this may happen when a user adds 2 policies like that. >>> But I think this's a problem that the user doesn't configure it well, >>> 'priority' should be set. >>> and this can not be avoided, also such as: >>> >>> policy A (mark.v = 0xff00, mark.m = 0x1000, priority = 1) >>> policy B (mark.v = 0x00ff, mark.m = 0x0011, priority = 1) >>> >>> try with 0x12341011 >>> >>> So just be it, let users decide. >> >> Ok, this make sense. > Thanks Yuehaibing, it's good we're on the same page now. > > Just realized the patch I created above won't work for the case: > > policy A (mark.v = 0x10, mark.m = 0, priority = 1) > policy B (mark.v = 0x1, mark.m = 0, priority = 2) > policy C (mark.v = 0x10, mark.m = 0, priority = 2) > > when policy C is being added, the warning still occurs. Do you means this: policy A (mark.v = 0x10, mark.m = 0, priority = 1) policy B (mark.v = 0x10, mark.m = 1, priority = 2) policy C (mark.v = 0x10, mark.m = 0, priority = 2) > > So I will just check value and priority: > - u32 mark = policy->mark.v & policy->mark.m; > - > - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) > - return true; > - > - if ((mark & pol->mark.m) == pol->mark.v && > + if (policy->mark.v == pol->mark.v && > policy->priority == pol->priority) > return true; > > This allows two policies like this exist: > > policy A (mark.v = 0x10, mark.m = 0, priority = 1) > policy C (mark.v = 0x10, mark.m = 0, priority = 2) > > But I don't think it's a problem. Agreed. > > . >