From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Aaron Conole <aconole@redhat.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
Sunil Kumar Kori <skori@marvell.com>,
"Burakov, Anatoly" <anatoly.burakov@intel.com>,
Chas Williams <chas3@att.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>,
"David Marchand" <david.marchand@redhat.com>
Subject: Re: [dpdk-dev] [PATCH v3 1/4] ip_frag: ensure minimum v4 fragmentation length
Date: Wed, 8 Apr 2020 12:37:24 +0000 [thread overview]
Message-ID: <SN6PR11MB255835CC8D01ECE6C3B32B6D9AC00@SN6PR11MB2558.namprd11.prod.outlook.com> (raw)
In-Reply-To: <f7td08j9m0a.fsf@dhcp-25.97.bos.redhat.com>
> >> >> The IPv4 specification says that each fragment must at least the size of
> >> >> an IP header plus 8 octets. When attempting to run ipfrag using a
> >> >> smaller size, the fragment library will return successful completion,
> >> >> even though it is a violation of RFC791 (and updates).
> >> >>
> >> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> >> ---
> >> >> lib/librte_ip_frag/rte_ipv4_fragmentation.c | 6 ++++++
> >> >> 1 file changed, 6 insertions(+)
> >> >>
> >> >> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> >> >> index 9e9f986cc5..4baaf6355c 100644
> >> >> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> >> >> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c
> >> >> @@ -76,6 +76,12 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_in,
> >> >> uint16_t fragment_offset, flag_offset, frag_size;
> >> >> uint16_t frag_bytes_remaining;
> >> >>
> >> >> + /*
> >> >> + * Ensure the IP fragmentation size is at least iphdr length + 8 octets
> >> >> + */
> >> >> + if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + 8*sizeof(char))))
> >> >> + return -EINVAL;
> >> >> +
> >> >
> >> > Same comment as for ipv6: ipv4 min MTU is 68B.
> >>
> >> I can change it. I suspected that if I went with 68 here and 1280 in
> >> the v6 code, I would get pushback, but I should have just submitted it
> >> that way to begin.
> >>
> >> > Why do we need extra checking here?
> >>
> >> These are error conditions to submit to fragmentation module. Someone
> >> needs to do the check - either it is done in the application or the
> >> library. If the library doesn't, and the application writer doesn't
> >> know they must write these checks (it isn't documented anywhere), then
> >> we get non compliant behavior. By putting it in the library, we can
> >> clearly signal the application writer such a case has occurred.
> >>
> >> Should we not do error checking?
> >
> > It depends I think...
> > In many data-path functions we skip parameter checking.
> > These fragment() functions are data-path too.
> > Agree, it is not stated clearly in these functions formal comments,
> > as it should be.
>
> I'll add documentation as another patch.
>
> > After another thought - these functions are quite heavy-weighed anyway,
> > so probably formal parameter checking, something like:
> > if (pkt_in == NULL || pkts_out == NULL || pool_direct == NULL ||
> > pool_indirect == NULL || mtu < MIN_MTU)
> > return -EINVAL;
> >
> > wouldn't introduce any real slowdown.
>
> Agreed.
>
> > About more intense checking - like parsing all extension
> > headers, etc. - I think it would be too much overhead.
> > Again for that there is a special function that user can call directly:
> > rte_ipv6_frag_get_ipv6_fragment_header
> > (though its current implementation also checks only first extension header).
> > So, I think we just need to document that it is a user responsibility to
> > provide not fragmented yet packet, without any pre-fragment headers.
>
> Makes sense. Then again, the v6 frag code will need to preserve many of
> the headers anyway, so since we have to read them, maybe it makes
> sense to do the check here anyway. WDYT?
If we want to make this function fully compliant to what rfc8200 says,
then yes - extra changes is required in current implementation:
1. somehow obtain information about pre-fragment extensions length
2. use info from #1 to put fragment header at proper location.
And extra testing of course.
Probably safer and easier, for that patch just add formal parameter checking.
And if you feel like that - have the hard part as a separate patch.
>
> > Konstantin
> >
> >>
> >> >> /*
> >> >> * Ensure the IP payload length of all fragments is aligned to a
> >> >> * multiple of 8 bytes as per RFC791 section 2.3.
> >> >> --
> >> >> 2.25.1
next prev parent reply other threads:[~2020-04-08 12:37 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-31 16:07 [dpdk-dev] [PATCH 0/4] ip_frag: add a unit test for fragmentation Aaron Conole
2020-03-31 16:07 ` [dpdk-dev] [PATCH 1/4] ip_frag: ensure minimum v4 fragmentation length Aaron Conole
2020-03-31 16:07 ` [dpdk-dev] [PATCH 2/4] ip_frag: ensure minimum v6 " Aaron Conole
2020-03-31 16:07 ` [dpdk-dev] [PATCH 3/4] ip_frag: ipv6 fragments must not be resubmitted to fragmentation Aaron Conole
2020-03-31 16:07 ` [dpdk-dev] [PATCH 4/4] ipfrag: add unit test case Aaron Conole
[not found] ` <20200331200715.13751-1-robot@bytheb.org>
2020-03-31 21:12 ` [dpdk-dev] |WARNING| pw67494 " Aaron Conole
2020-04-01 13:18 ` [dpdk-dev] [PATCH v2 0/4] ip_frag: add a unit test for fragmentation Aaron Conole
2020-04-01 13:18 ` [dpdk-dev] [PATCH v2 1/4] ip_frag: ensure minimum v4 fragmentation length Aaron Conole
2020-04-01 13:18 ` [dpdk-dev] [PATCH v2 2/4] ip_frag: ensure minimum v6 " Aaron Conole
2020-04-01 13:18 ` [dpdk-dev] [PATCH v2 3/4] ip_frag: ipv6 fragments must not be resubmitted to fragmentation Aaron Conole
2020-04-01 13:18 ` [dpdk-dev] [PATCH v2 4/4] ipfrag: add unit test case Aaron Conole
2020-04-01 18:39 ` [dpdk-dev] [PATCH v3 0/4] ip_frag: add a unit test for fragmentation Aaron Conole
2020-04-01 18:39 ` [dpdk-dev] [PATCH v3 1/4] ip_frag: ensure minimum v4 fragmentation length Aaron Conole
2020-04-07 11:10 ` Ananyev, Konstantin
2020-04-07 12:52 ` Aaron Conole
2020-04-07 14:14 ` Ananyev, Konstantin
2020-04-07 18:41 ` Aaron Conole
2020-04-08 12:37 ` Ananyev, Konstantin [this message]
2020-04-08 15:45 ` Aaron Conole
2020-04-01 18:39 ` [dpdk-dev] [PATCH v3 2/4] ip_frag: ensure minimum v6 " Aaron Conole
2020-04-07 10:48 ` Ananyev, Konstantin
2020-04-01 18:39 ` [dpdk-dev] [PATCH v3 3/4] ip_frag: ipv6 fragments must not be resubmitted to fragmentation Aaron Conole
2020-04-07 10:43 ` Ananyev, Konstantin
2020-04-07 12:40 ` Aaron Conole
2020-04-01 18:39 ` [dpdk-dev] [PATCH v3 4/4] ipfrag: add unit test case Aaron Conole
2020-04-04 15:58 ` Pavan Nikhilesh Bhagavatula
2020-04-15 17:25 ` [dpdk-dev] [PATCH v4 0/3] ip_frag: add a unit test for fragmentation Aaron Conole
2020-04-15 17:25 ` [dpdk-dev] [PATCH v4 1/3] ip_frag: ensure minimum v4 fragmentation length Aaron Conole
2020-04-17 11:52 ` Lukasz Wojciechowski
2020-04-15 17:25 ` [dpdk-dev] [PATCH v4 2/3] ip_frag: ensure minimum v6 " Aaron Conole
2020-04-17 11:52 ` Lukasz Wojciechowski
2020-04-15 17:25 ` [dpdk-dev] [PATCH v4 3/3] ipfrag: add unit test case Aaron Conole
2020-04-16 15:30 ` Lukasz Wojciechowski
2020-04-16 18:52 ` Aaron Conole
2020-04-17 13:14 ` [dpdk-dev] [PATCH v5 0/3] ip_frag: add a unit test for fragmentation Aaron Conole
2020-04-17 13:14 ` [dpdk-dev] [PATCH v5 1/3] ip_frag: ensure minimum v4 fragmentation length Aaron Conole
2020-04-20 12:50 ` Ananyev, Konstantin
2020-04-20 15:24 ` Aaron Conole
2020-04-17 13:14 ` [dpdk-dev] [PATCH v5 2/3] ip_frag: ensure minimum v6 " Aaron Conole
2020-04-20 12:53 ` Ananyev, Konstantin
2020-04-20 15:26 ` Aaron Conole
2020-04-20 15:43 ` Ananyev, Konstantin
2020-04-17 13:14 ` [dpdk-dev] [PATCH v5 3/3] ipfrag: add unit test case Aaron Conole
2020-04-17 14:14 ` Lukasz Wojciechowski
2020-04-20 16:03 ` Burakov, Anatoly
2020-04-20 17:34 ` Aaron Conole
2020-04-25 12:18 ` Thomas Monjalon
2020-04-20 19:25 ` [dpdk-dev] [PATCH v6 0/3] ip_frag: add a unit test for fragmentation Aaron Conole
2020-04-20 19:25 ` [dpdk-dev] [PATCH v6 1/3] ip_frag: ensure minimum v4 fragmentation length Aaron Conole
2020-04-21 11:04 ` Lukasz Wojciechowski
2020-04-20 19:25 ` [dpdk-dev] [PATCH v6 2/3] ip_frag: ensure minimum v6 " Aaron Conole
2020-04-21 11:04 ` Lukasz Wojciechowski
2020-04-20 19:25 ` [dpdk-dev] [PATCH v6 3/3] ipfrag: add unit test case Aaron Conole
2020-04-21 11:03 ` Lukasz Wojciechowski
2020-04-25 13:16 ` [dpdk-dev] [PATCH v6 0/3] ip_frag: add a unit test for fragmentation Thomas Monjalon
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=SN6PR11MB255835CC8D01ECE6C3B32B6D9AC00@SN6PR11MB2558.namprd11.prod.outlook.com \
--to=konstantin.ananyev@intel.com \
--cc=aconole@redhat.com \
--cc=anatoly.burakov@intel.com \
--cc=bruce.richardson@intel.com \
--cc=chas3@att.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=skori@marvell.com \
/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).