From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <bruce.richardson@intel.com> Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 4688E1B4FD for <dev@dpdk.org>; Tue, 16 Apr 2019 17:13:56 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Apr 2019 08:13:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,358,1549958400"; d="scan'208";a="141155592" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.220.103]) by fmsmga008.fm.intel.com with SMTP; 16 Apr 2019 08:13:53 -0700 Received: by (sSMTP sendmail emulation); Tue, 16 Apr 2019 16:13:52 +0100 Date: Tue, 16 Apr 2019 16:13:51 +0100 From: Bruce Richardson <bruce.richardson@intel.com> To: Stephen Hemminger <stephen@networkplumber.org> Cc: Ferruh Yigit <ferruh.yigit@intel.com>, dev@dpdk.org, Chas Williams <chas3@att.com>, "John W. Linville" <linville@tuxdriver.com> Message-ID: <20190416151351.GA1875@bricha3-MOBL.ger.corp.intel.com> References: <20190408160419.7409-1-stephen@networkplumber.org> <20190408164112.12471-1-stephen@networkplumber.org> <7e03b000-f4d4-71c3-5978-8a8623d7ace5@intel.com> <20190412150833.63c41806@shemminger-XPS-13-9360> <3dff5ba9-3965-be8d-7bd5-d8504a66c130@intel.com> <20190416094212.GA1865@bricha3-MOBL.ger.corp.intel.com> <20190416080336.3b8e16a6@xps13.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190416080336.3b8e16a6@xps13.lan> User-Agent: Mutt/1.11.4 (2019-03-13) Subject: Re: [dpdk-dev] [PATCH v2] net/af_packet: fix vlan_insert corruption X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Tue, 16 Apr 2019 15:13:56 -0000 On Tue, Apr 16, 2019 at 08:03:36AM -0700, Stephen Hemminger wrote: > On Tue, 16 Apr 2019 10:42:13 +0100 > Bruce Richardson <bruce.richardson@intel.com> wrote: > > > On Tue, Apr 16, 2019 at 10:37:07AM +0100, Ferruh Yigit wrote: > > > On 4/12/2019 11:08 PM, Stephen Hemminger wrote: > > > > On Fri, 12 Apr 2019 17:28:17 +0100 > > > > Ferruh Yigit <ferruh.yigit@intel.com> wrote: > > > > > > > >> On 4/8/2019 5:41 PM, Stephen Hemminger wrote: > > > >>> If the af_packet transmit is sending a VLAN packet, > > > >>> and the transmit path to the kernel os full, then it would > > > >>> mismanage the outgoing mbuf. The original mbuf would end up > > > >>> being freed twice, once by AF_PACKET PMD and once by caller. > > > >> > > > >> This comment is valid with your new patch [1] that updates 'rte_vlan_insert()' > > > >> to duplicate the mbuf, right? > > > >> > > > >> That patch looks like won't make the release, so I suggest this one wait that > > > >> patch, although this is harmless on its own, commit log is misleading. > > > >> > > > >> [1] > > > >> https://patches.dpdk.org/patch/51870/ > > > > > > > > It was always true, even with existing vlan_insert. > > > > Existing vlan_insert has issues if it ever creates a clone packet. > > > > Existing vlan_insert can duplicate mbuf through clone > > > > > > > > > > Right, existing vlan_insert has same issue on af_packet. > > > > > > But, should vlan_insert try to duplicate the mbuf when it is shared, does it > > > worth the complexity it brings? And when that support removed this patch won't > > > be needed. > > > > I don't think vlan insert or other mbuf manipulation APIs should be > > checking for shared state or not - that's the job of the app. We could have > > cases where the user does want to modify a shared mbuf. > > > > Regards, > > /Bruce > > The vlan_insert code is called on transmit, and there are lots of > cases where a transmit mbuf might be shared (like TCP stack). And in that > case inserting the vlan must be non-destructive to the original mbuf. > > Whether you want to push the problem to the driver or do it in the > library, it is still a problem. Yes, I agree it's a problem. I'd prefer see it done in the driver than in the library, since it's higher in the SW stack and has more context information as to what is safe or not. /Bruce. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <dev-bounces@dpdk.org> Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 2A993A00E6 for <public@inbox.dpdk.org>; Tue, 16 Apr 2019 17:13:59 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1C9C91B4FE; Tue, 16 Apr 2019 17:13:58 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 4688E1B4FD for <dev@dpdk.org>; Tue, 16 Apr 2019 17:13:56 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Apr 2019 08:13:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,358,1549958400"; d="scan'208";a="141155592" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.220.103]) by fmsmga008.fm.intel.com with SMTP; 16 Apr 2019 08:13:53 -0700 Received: by (sSMTP sendmail emulation); Tue, 16 Apr 2019 16:13:52 +0100 Date: Tue, 16 Apr 2019 16:13:51 +0100 From: Bruce Richardson <bruce.richardson@intel.com> To: Stephen Hemminger <stephen@networkplumber.org> Cc: Ferruh Yigit <ferruh.yigit@intel.com>, dev@dpdk.org, Chas Williams <chas3@att.com>, "John W. Linville" <linville@tuxdriver.com> Message-ID: <20190416151351.GA1875@bricha3-MOBL.ger.corp.intel.com> References: <20190408160419.7409-1-stephen@networkplumber.org> <20190408164112.12471-1-stephen@networkplumber.org> <7e03b000-f4d4-71c3-5978-8a8623d7ace5@intel.com> <20190412150833.63c41806@shemminger-XPS-13-9360> <3dff5ba9-3965-be8d-7bd5-d8504a66c130@intel.com> <20190416094212.GA1865@bricha3-MOBL.ger.corp.intel.com> <20190416080336.3b8e16a6@xps13.lan> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: <20190416080336.3b8e16a6@xps13.lan> User-Agent: Mutt/1.11.4 (2019-03-13) Subject: Re: [dpdk-dev] [PATCH v2] net/af_packet: fix vlan_insert corruption X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> Message-ID: <20190416151351.4CKEiFtROd4_vGCa1qhYYasvMWPwrtrIrvcysQJTsYY@z> On Tue, Apr 16, 2019 at 08:03:36AM -0700, Stephen Hemminger wrote: > On Tue, 16 Apr 2019 10:42:13 +0100 > Bruce Richardson <bruce.richardson@intel.com> wrote: > > > On Tue, Apr 16, 2019 at 10:37:07AM +0100, Ferruh Yigit wrote: > > > On 4/12/2019 11:08 PM, Stephen Hemminger wrote: > > > > On Fri, 12 Apr 2019 17:28:17 +0100 > > > > Ferruh Yigit <ferruh.yigit@intel.com> wrote: > > > > > > > >> On 4/8/2019 5:41 PM, Stephen Hemminger wrote: > > > >>> If the af_packet transmit is sending a VLAN packet, > > > >>> and the transmit path to the kernel os full, then it would > > > >>> mismanage the outgoing mbuf. The original mbuf would end up > > > >>> being freed twice, once by AF_PACKET PMD and once by caller. > > > >> > > > >> This comment is valid with your new patch [1] that updates 'rte_vlan_insert()' > > > >> to duplicate the mbuf, right? > > > >> > > > >> That patch looks like won't make the release, so I suggest this one wait that > > > >> patch, although this is harmless on its own, commit log is misleading. > > > >> > > > >> [1] > > > >> https://patches.dpdk.org/patch/51870/ > > > > > > > > It was always true, even with existing vlan_insert. > > > > Existing vlan_insert has issues if it ever creates a clone packet. > > > > Existing vlan_insert can duplicate mbuf through clone > > > > > > > > > > Right, existing vlan_insert has same issue on af_packet. > > > > > > But, should vlan_insert try to duplicate the mbuf when it is shared, does it > > > worth the complexity it brings? And when that support removed this patch won't > > > be needed. > > > > I don't think vlan insert or other mbuf manipulation APIs should be > > checking for shared state or not - that's the job of the app. We could have > > cases where the user does want to modify a shared mbuf. > > > > Regards, > > /Bruce > > The vlan_insert code is called on transmit, and there are lots of > cases where a transmit mbuf might be shared (like TCP stack). And in that > case inserting the vlan must be non-destructive to the original mbuf. > > Whether you want to push the problem to the driver or do it in the > library, it is still a problem. Yes, I agree it's a problem. I'd prefer see it done in the driver than in the library, since it's higher in the SW stack and has more context information as to what is safe or not. /Bruce.