From: "Varghese, Vipin" <vipin.varghese@intel.com>
To: "Varghese, Vipin" <vipin.varghese@intel.com>,
Ophir Munk <ophirmu@mellanox.com>, "dev@dpdk.org" <dev@dpdk.org>,
"pascal.mazon@6wind.com" <pascal.mazon@6wind.com>,
"Yigit, Ferruh" <ferruh.yigit@intel.com>,
Thomas Monjalon <thomas@monjalon.net>,
Olga Shern <olgas@mellanox.com>,
Shahaf Shuler <shahafs@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH 1/2] net/tap: add tun support
Date: Mon, 23 Apr 2018 12:58:47 +0000 [thread overview]
Message-ID: <4C9E0AB70F954A408CC4ADDBF0F8FA7D4D1E3338@BGSMSX101.gar.corp.intel.com> (raw)
In-Reply-To: <4C9E0AB70F954A408CC4ADDBF0F8FA7D4D1DFD5A@BGSMSX101.gar.corp.intel.com>
Hi Ophir,
Can you help me with the investigation with the following information?
1) The kernel or distro in which the TAP proto flag set breaks the logic?
2) Is the above still valid even after applying the patch ' https://dpdk.org/dev/patchwork/patch/37986/'?
Note: I am testing with 3.13.0, 4.4.0 and 4.13.0.
Thanks
Vipin Varghese
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Varghese, Vipin
> Sent: Saturday, April 21, 2018 8:40 PM
> To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org;
> pascal.mazon@6wind.com; Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas
> Monjalon <thomas@monjalon.net>; Olga Shern <olgas@mellanox.com>;
> Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH 1/2] net/tap: add tun support
>
> Hi Ophir,
>
> <Snip>
>
> > Hi Vipin,
> > I missed your point:
> > You claim that TAP should work regardless of any pi.proto values.
> > Can you confirm that for ALL kernels versions (past and future)?
>
> I have tested with 3.13.0 , 4.4.0 with patch fix.
>
> >
> > > -----Original Message-----
> > > From: Ophir Munk
> > > Sent: Saturday, April 21, 2018 12:49 AM
> > > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> > > pascal.mazon@6wind.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> > > <olgas@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> > > Subject: RE: [dpdk-dev] [PATCH 1/2] net/tap: add tun support
> > >
> > > Hi Vipin,
> > >
> > > Please find comments inline.
> > >
> > > > -----Original Message-----
> > > > From: Varghese, Vipin [mailto:vipin.varghese@intel.com]
> > > > Sent: Friday, April 13, 2018 6:18 AM
> > > > To: Ophir Munk <ophirmu@mellanox.com>; dev@dpdk.org;
> > > > pascal.mazon@6wind.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > > Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> > > > <olgas@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> > > > Subject: RE: [dpdk-dev] [PATCH 1/2] net/tap: add tun support
> > > >
>
> <Snip>
>
> > > > > 1. Accessing the first byte here assumes it is the first IP
> > > > > header byte (layer 3) which is correct for TUN.
> > > > > For TAP however the first byte belongs to Ethernet destination
> > > > > address (layer 2).
> > > > > Please explain how this logic will work for TAP.
> > > >
> > > > Based on linux code base '/driver/net/tap.c' and '/driver/net/tun.c'
> > > > from 3.13. to 4.16,
> > > >
> > > > Please find my observation below
> > > > 1. File: tun.c, function: tun_get_user, check for 'tun->flags &
> > > > TUN_TYPE_MASK' is done and if non ip is taken counter 'rx_dropped'
> > > > is updated.
> > > > 2. File: tap.c, there are no checks for 'tap->flags' for IFF_NO_PI
> > > > in rx data path. Counter 'rx_dropped' is updated in 'tap_handle_frame'.
> > > >
> > >
> > > I understand that in kernel implementation there is no check for
> > > tap->flags in file tap.c, however I think there is a bug in dpdk
> > > tap->rte_eth_tap.c
> > file.
> > > Please find below an example which demonstrates this claim.
> > >
> > > > Please find my reasoning below
> > > > 1. First approach was to have separate function for tap and tun TX and
> RX.
> > > > But this will introduce code duplication, hence reworked the code
> > > > as
> > > above.
> > >
> > > I agree. Avoiding code duplication is a good approach.
> > >
> > > > 2. During my internal testing assigning dummy value for protocol
> > > > field in TAP packets, did not show a difference in behaviour. May
> > > > be there are some specific cases this failing.
> > > >
> > > > If there difference in behaviour, can please share the same?
> > > >
> > >
> > > Please consider the following example:
> > > I am running testpmd with a TAP device, --forward-mode=csum.
> > > I am injecting a TCP packet, which is forwarded back (mac addresses
> > > swapped) to the sender.
> > > Using gdb I set a breakpoint at pmd_tx_burst() in file rte_eth_tap.c
> > >
> > > Looking at the following code inside pmd_tx_burst():
> > >
> > > 527 char *buff_data = rte_pktmbuf_mtod(seg, void *);
> > > 528 j = (*buff_data & 0xf0);
> > > 529 pi.proto = (j == 0x40) ? 0x0008 :
> > > 530 (j == 0x60) ? 0xdd86 : 0x00;
> > >
> > > I am printing the first 20 bytes of buff_data in line 527:
> > >
> > > (gdb) p/x *(unsigned char *)buff_data@20
> > > $3 = {0x0, 0x25, 0x88, 0x10, 0x66, 0x2, 0xf4, 0x52, 0x14, 0x7a,
> > > 0x59, 0x81, 0x8, 0x0, 0x45, 0x0, 0x4, 0xdf, 0x0, 0x1}
> > >
> > > The gdb printout refers to:
> > > 6 bytes of destination MAC address: 0x0, 0x25, 0x88, 0x10, 0x66, 0x2
> > > 6 bytes of source MAC address: 0xf4, 0x52, 0x14, 0x7a, 0x59, 0x81
> > > 2 bytes of Ethernet type: 0x8, 0x0 - (IPv4) IP header starting with 0x45, ...
> > > which is the byte (0x45) that "j" should have looked at
> > >
> > > In the case of TAP - buff_data starts with the destination MAC
> > > address of the sender (0x0, ...).
> > > The code in line 528 expects that buff_data would start with an IP
> > > header protocol (e.g. 0x45), but it is not the case for TAP.
> > > In my case j=0x0 (line 528) which is harmless (as it ends up with
> > > setting pi.proto=0x00, which is correct for TAP).
> > > However, if the sender had an Intel NIC - the destination MAC
> > > address could have started with:
> > > $3 = {0x40, 0x25, 0xC2, ...
> > > Or-
> > > $3 = {0x64, 0xD4, 0xDA, ...
> > >
> > > as 4025C2 and 64D4DA are reserved prefixes for Intel Ethernet MAC
> > > addresses, see: http://www.coffer.com/mac_find/?string=intel
> > >
> > > In this case pi.proto could end up with 0x0008 or 0xdd86 instead of
> > > 0x0 as expected for TAP.
> > >
> > > I hope that this example clarifies the bug I am referring to.
> > >
>
> Thanks for sharing detailed example overview. But as you mentioned this will
> break ' 4025C2' and ' 64D4DA', This will not solve for the correction patch '
> https://dpdk.org/dev/patchwork/patch/37986/'.
>
> Only choice left is separate tx_burst for TAP and TUN PMD, as we do not
> want to check PMD type on each call.
>
> Questions:
> 1) Is this ok to split tx_burst and have redundant code?
> 2) Does applications transparently send packets coming from Physical NIC to
> TAP interface? Does not the application Modifies the DEST MAC addr to TAP
> interface?
>
> > > > >
> > > > > 2. If the first TUN byte contains 0x2X (which is neither IPv4
> > > > > nor
> > > > > IPv6) it will end up by setting ip.proto as 0xdd86.
> > > > > Please explain how this logic will work for non-IP packets in
> > > > > TUN
> > > >
> > > > I see your point. You are correct about this. Thanks for pointing
> > > > out, may I send correction for this as
> > > >
> > > > """
> > > > - if (j & (0x40 | 0x60))
> > > > - pi.proto = (j == 0x40) ? 0x0008 : 0xdd86;
> > > > + pi.proto = (j == 0x40) ? 0x0008 :
> > > > + (j == 0x60) ? 0xdd86 :
> > > > + 0x00;
> > > > """
next prev parent reply other threads:[~2018-04-23 12:58 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-20 23:12 [dpdk-dev] [PATCH v1] " Vipin Varghese
2018-02-22 12:22 ` [dpdk-dev] [dpdk-dev,v1] " Pascal Mazon
2018-02-26 11:01 ` Varghese, Vipin
2018-02-26 6:15 ` [dpdk-dev] [PATCH v2] " Vipin Varghese
2018-02-27 13:06 ` Pascal Mazon
2018-04-02 21:37 ` [dpdk-dev] [PATCH 1/2] " Vipin Varghese
2018-04-02 21:37 ` [dpdk-dev] [PATCH 2/2] net/tap: add tun log and documnetation Vipin Varghese
2018-04-03 8:27 ` Pascal Mazon
2018-04-03 10:05 ` Ferruh Yigit
2018-04-03 10:05 ` Ferruh Yigit
2018-04-03 10:53 ` Pascal Mazon
2018-04-06 17:11 ` [dpdk-dev] [PATCH 1/2] net/tap: add tun support Ferruh Yigit
2018-04-06 17:19 ` Ferruh Yigit
2018-04-12 11:49 ` Ophir Munk
2018-04-13 3:18 ` Varghese, Vipin
2018-04-20 21:49 ` Ophir Munk
2018-04-20 21:58 ` Ophir Munk
2018-04-21 15:09 ` Varghese, Vipin
2018-04-23 12:58 ` Varghese, Vipin [this message]
2018-04-23 15:42 ` Ferruh Yigit
2018-05-03 5:59 ` Varghese, Vipin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4C9E0AB70F954A408CC4ADDBF0F8FA7D4D1E3338@BGSMSX101.gar.corp.intel.com \
--to=vipin.varghese@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=olgas@mellanox.com \
--cc=ophirmu@mellanox.com \
--cc=pascal.mazon@6wind.com \
--cc=shahafs@mellanox.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).