Date: Wed, 6 Apr 2011 18:20:12 GMT From: Karim Fodil-Lemelin <fodillemlinkarim@gmail.com> To: freebsd-ipfw@FreeBSD.org Subject: Re: kern/156180 Message-ID: <201104061820.p36IKCg3002267@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/156180; it has been noted by GNATS. From: Karim Fodil-Lemelin <fodillemlinkarim@gmail.com> To: Gleb Smirnoff <glebius@FreeBSD.org> Cc: Subject: Re: kern/156180 Date: Wed, 6 Apr 2011 14:07:08 -0400 --000e0cd5d1826dc62504a043dcf6 Content-Type: text/plain; charset=ISO-8859-1 Hi Gleb, 2011/4/6 Gleb Smirnoff <glebius@freebsd.org> > Hi, Karim! > > On Wed, Apr 06, 2011 at 10:36:46AM -0400, Karim Fodil-Lemelin wrote: > K> Thanks for the patch it does work in FBSD although it does not work in > my > K> setup since I have extended TCP option checking into another ipfw action > and > K> while I could add the check you've proposed for tcpop_match I would > prefer a > K> more generic approach where the m_pullup call is done for all TCP > packets > K> with options (basically in the case IPPROTO_TCP). > K> > K> The rationale behind this is such that there is a guarantee that > tcpop_match > K> will work but also that any future extensions based on TCP options would > K> also work saving the hard to debug situation that a missing call to > m_pullup > K> can create. > > Currently almost all TCP traffic has TCP options. And currently most, > I suppose > 90%, installations, that use ipfw(4) do not have rules with > 'tcpoptions' keyword. So, we would add extra pullup, that is not needed > in most cases and may have a performance impact. > In my case even if all packets have at least a timestamp option or SACK the actual m_pullup was only called once every 10s of thousands packet as most of the time the lower layers hands out perfectly contiguous headers. What I've found is whenever a netgraph node or some other decapsulation mechanisms that tends to modify headers and due to its internal operations needs to somewhat break down the mbuf chain, the m_pullup call might be needed due to trailing options being in another mbuf. Most other times the headers are just fine and thats what made this problem hard to find in the first place. So I don't think there is a significant performance hit but I do see your point (see my point on multiple TCP microinstructions below). > In case of future extensions the hard to debug situation won't happen > if a developer analyses the function he modifies thoroughly. > One could assume that adding an action to ipfw should be relatively straightforward and the mechanics of doing sanity checks and what not was already handled by the top half of the function. Although I'm sure that with the call to PULLUP_LEN in tcpopt_match developers basing their code on it will not miss it ;) I can understand that since TCP option filtering isn't used much by the community its better to keep its specific processing isolated from IP although that approach has its drawback. You can imagine that if multiple TCP actions are chained (as microinstructions) together (and so are the calls to PULLUP_LEN) the call overhead becomes quickly a performance issue. > > So, can you please confirm, that if you adding this string > > PULLUP_LEN(hlen, ulp, (TCP(ulp)->th_off << 2)); > > into your new ipfw action, solves the discussed problem? > Unfortunately I won't be able to test this exact code, as I mentionned earlier, since it is not applicable to our solution (btw the problem would show up once every 2 days or so). My PR was really just a mere pointer to an issue I have encountered and I perfectly understand the context of your solution and its applicability. You see myself in the impossibility to confirm with testing your proposed patch. If that can be a consolation my first fix was to add a call to m_pullup in every TCP action we had and it worked just fine although the final solution was to integrate it higher up in the function (before the microinstruction loop). Though, I do have to admit I prefer your macro integration to what I've proposed since it 'blends in' better with the previous implementation. Best regards, Karim. --000e0cd5d1826dc62504a043dcf6 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Hi Gleb,<br><br><div class=3D"gmail_quote">2011/4/6 Gleb Smirnoff <span dir= =3D"ltr"><<a href=3D"mailto:glebius@freebsd.org">glebius@freebsd.org</a>= ></span><br><blockquote class=3D"gmail_quote" style=3D"border-left: 1px = solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> =A0Hi, Karim!<br> <br> On Wed, Apr 06, 2011 at 10:36:46AM -0400, Karim Fodil-Lemelin wrote:<br> K> Thanks for the patch it does work in FBSD although it does not work i= n my<br> K> setup since I have extended TCP option checking into another ipfw act= ion and<br> K> while I could add the check you've proposed for tcpop_match I wou= ld prefer a<br> K> more generic approach where the m_pullup call is done for all TCP pac= kets<br> K> with options (basically in the case IPPROTO_TCP).<br> K><br> K> The rationale behind this is such that there is a guarantee that tcpo= p_match<br> K> will work but also that any future extensions based on TCP options wo= uld<br> K> also work saving the hard to debug situation that a missing call to m= _pullup<br> K> can create.<br> <br> Currently almost all TCP traffic has TCP options. And currently most,<br> I suppose > 90%, installations, that use ipfw(4) do not have rules with<= br> 'tcpoptions' keyword. So, we would add extra pullup, that is not ne= eded<br> in most cases and may have a performance impact.<br></blockquote><div><br>I= n my case even if all packets have at least a timestamp option or SACK the = actual m_pullup was only called once every 10s of thousands packet as most = of the time the lower layers hands out perfectly contiguous headers.<br> <br>What I've found is whenever a netgraph node or some other decapsula= tion mechanisms that tends to modify headers and due to its internal operat= ions needs to somewhat break down the mbuf chain, the m_pullup call might b= e needed due to trailing options being in another mbuf. Most other times th= e headers are just fine and thats what made this problem hard to find in th= e first place.<br> <br>So I don't think there is a significant performance hit but I do se= e your point (see my point on multiple TCP microinstructions below).<br><br= ></div><blockquote class=3D"gmail_quote" style=3D"border-left: 1px solid rg= b(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <br> In case of future extensions the hard to debug situation won't happen<b= r> if a developer analyses the function he modifies thoroughly.<br></blockquot= e><div>=A0</div><div>One could assume that adding an action to ipfw should = be relatively straightforward and the mechanics of doing sanity checks and = what not was already handled by the top half of the function. Although I= 9;m sure that with the call to PULLUP_LEN in tcpopt_match developers basing= their code on it will not miss it ;)<br> <br>I can understand that since TCP option filtering isn't used much by= the community its better to keep its specific processing isolated from IP = although that approach has its drawback. You can imagine that if multiple T= CP actions are chained (as microinstructions) together (and so are the call= s to PULLUP_LEN) the call overhead becomes quickly a performance issue.<br> =A0</div><blockquote class=3D"gmail_quote" style=3D"border-left: 1px solid = rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <br> So, can you please confirm, that if you adding this string<br> <br> PULLUP_LEN(hlen, ulp, (TCP(ulp)->th_off << 2));<br> <br> into your new ipfw action, solves the discussed problem?<br></blockquote></= div><br>Unfortunately I won't be able to test this exact code, as I men= tionned earlier, since it is not applicable to our solution (btw the proble= m would show up once every 2 days or so). My PR was really just a mere poin= ter to an issue I have encountered and I perfectly understand the context o= f your solution and its applicability. You see myself in the impossibility = to confirm with testing your proposed patch.<br> <br>If that can be a consolation my first fix was to add a call to m_pullup= in every TCP action we had and it worked just fine although the final solu= tion was to integrate it higher up in the function (before the microinstruc= tion loop). Though, I do have to admit I prefer your macro integration to w= hat I've proposed since it 'blends in' better with the previous= implementation.<br> <br>Best regards,<br><br>Karim.<br> --000e0cd5d1826dc62504a043dcf6--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201104061820.p36IKCg3002267>