| Message ID | 20170919003904.5124-10-tom@quantonium.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | gtp: Additional feature support | expand |
On 19/09/17 02:38, Tom Herbert wrote: > Add new configuration of GTP interfaces that allow specifying a port to > listen on (as opposed to having to get sockets from a userspace control > plane). This allows GTP interfaces to be configured and the data path > tested without requiring a GTP-C daemon. This would imply that you can have multiple independent GTP sockets on the same IP address.That is not permitted by the GTP specifications. 3GPP TS 29.281, section 4.3 states clearly that there is "only" one GTP entity per IP address.A PDP context is defined by the destination IP and the TEID. The destination port is not part of the identity of a PDP context. Even the source IP and source port are not part of the tunnel identity. This makes is possible to send traffic from a new SGSN/SGW during handover before the control protocol has announced the handover. At this point the usual response is: THAT IS NOT SAFE. Yes, GTP has been designed for cooperative networks only and should not be used on hostile/unsecured networks. On the sending side, using multiple ports is permitted as long as the default GTP port is always able to receive incoming messages. Andreas [...]
On Wed, Sep 20, 2017 at 8:27 AM, Andreas Schultz <aschultz@tpip.net> wrote: > On 19/09/17 02:38, Tom Herbert wrote: >> >> Add new configuration of GTP interfaces that allow specifying a port to >> listen on (as opposed to having to get sockets from a userspace control >> plane). This allows GTP interfaces to be configured and the data path >> tested without requiring a GTP-C daemon. > > > This would imply that you can have multiple independent GTP sockets on the > same IP address.That is not permitted by the GTP specifications. 3GPP TS > 29.281, section 4.3 states clearly that there is "only" one GTP entity per > IP address.A PDP context is defined by the destination IP and the TEID. The > destination port is not part of the identity of a PDP context. > We are in no way trying change GTP, if someone runs this in a real GTP network then they need to abide by the specification. However, there is nothing inconsistent and it breaks nothing if someone wishes to use different port numbers in their own private network for testing or development purposes. Every other UDP application that has assigned port number allows configurable ports, I don't see that GTP is so special that it should be an exception. Tom
On 20/09/17 17:57, Tom Herbert wrote: > On Wed, Sep 20, 2017 at 8:27 AM, Andreas Schultz <aschultz@tpip.net> wrote: >> On 19/09/17 02:38, Tom Herbert wrote: >>> >>> Add new configuration of GTP interfaces that allow specifying a port to >>> listen on (as opposed to having to get sockets from a userspace control >>> plane). This allows GTP interfaces to be configured and the data path >>> tested without requiring a GTP-C daemon. >> >> >> This would imply that you can have multiple independent GTP sockets on the >> same IP address.That is not permitted by the GTP specifications. 3GPP TS >> 29.281, section 4.3 states clearly that there is "only" one GTP entity per >> IP address.A PDP context is defined by the destination IP and the TEID. The >> destination port is not part of the identity of a PDP context. >> > We are in no way trying change GTP, if someone runs this in a real GTP > network then they need to abide by the specification. However, there > is nothing inconsistent and it breaks nothing if someone wishes to use > different port numbers in their own private network for testing or > development purposes. Every other UDP application that has assigned > port number allows configurable ports, I don't see that GTP is so > special that it should be an exception. GTP isn't special, I just don't like to have testing only features in there when the same goal can be reached without having to add extra stuff. Adding code that is not going to be useful in real production setups (or in this case would even break production setups when enabled accidentally) makes the implementation more complex than it needs to be. You can always add multiple IP's to your test system and have the same effect without having to change the ports. Regards Andreas > > Tom >
On Wed, Sep 20, 2017 at 9:07 AM, Andreas Schultz <aschultz@tpip.net> wrote: > > > On 20/09/17 17:57, Tom Herbert wrote: >> >> On Wed, Sep 20, 2017 at 8:27 AM, Andreas Schultz <aschultz@tpip.net> >> wrote: >>> >>> On 19/09/17 02:38, Tom Herbert wrote: >>>> >>>> >>>> Add new configuration of GTP interfaces that allow specifying a port to >>>> listen on (as opposed to having to get sockets from a userspace control >>>> plane). This allows GTP interfaces to be configured and the data path >>>> tested without requiring a GTP-C daemon. >>> >>> >>> >>> This would imply that you can have multiple independent GTP sockets on >>> the >>> same IP address.That is not permitted by the GTP specifications. 3GPP TS >>> 29.281, section 4.3 states clearly that there is "only" one GTP entity >>> per >>> IP address.A PDP context is defined by the destination IP and the TEID. >>> The >>> destination port is not part of the identity of a PDP context. >>> >> We are in no way trying change GTP, if someone runs this in a real GTP >> network then they need to abide by the specification. However, there >> is nothing inconsistent and it breaks nothing if someone wishes to use >> different port numbers in their own private network for testing or >> development purposes. Every other UDP application that has assigned >> port number allows configurable ports, I don't see that GTP is so >> special that it should be an exception. > > > GTP isn't special, I just don't like to have testing only features in there > when the same goal can be reached without having to add extra stuff. Adding > code that is not going to be useful in real production setups (or in this > case would even break production setups when enabled accidentally) makes the > implementation more complex than it needs to be. Well, you could make the same argument that allowing GTP to configured as standalone interface is a problem since GTP is only allowed to be with used with GTP-C. But, then we have something in the kernel that the community is expected to support, but requires jumping through a whole bunch of hoops just to run a simple netperf. The more that patches and features look like other things in the kernel that are already well established, the better the chances we can accept them and support them. It's probably a natural consequence of any large open source project, so sometimes it's worth the effort to add a few lines of complexity to get the benefits of community contribution and support. Tom
Hi Tom, On Wed, Sep 20, 2017 at 09:24:07AM -0700, Tom Herbert wrote: > On Wed, Sep 20, 2017 at 9:07 AM, Andreas Schultz <aschultz@tpip.net> wrote: > > GTP isn't special, I just don't like to have testing only features in there > > when the same goal can be reached without having to add extra stuff. Adding > > code that is not going to be useful in real production setups (or in this > > case would even break production setups when enabled accidentally) makes the > > implementation more complex than it needs to be. > > Well, you could make the same argument that allowing GTP to configured > as standalone interface is a problem since GTP is only allowed to be > with used with GTP-C. But, then we have something in the kernel that > the community is expected to support, but requires jumping through a > whole bunch of hoops just to run a simple netperf. "A whole bunch of hoops" without your new interface would consist of running a single command-line program that is supplied with libgtpnl. This is not a complete 3GPP network, but a simple libmnl-based helper library with no other depenencies. I'm not neccessarily against introducing features like the 'standalone interface configuration'. However, we must make sure that any significant new feature contributions like IPv6 are tested in a "realistic setup" and not just using those 'interfaces added for easy development'. Also, I would argue those 'interfaces added for easy deveopment/benchmarking' should probably be clearly marked as such to avoid raising the impression that this is what leads to a standard-conforming / production-type setup.
On Wed, Sep 20, 2017 at 5:13 PM, Harald Welte <laforge@gnumonks.org> wrote: > Hi Tom, > > On Wed, Sep 20, 2017 at 09:24:07AM -0700, Tom Herbert wrote: >> On Wed, Sep 20, 2017 at 9:07 AM, Andreas Schultz <aschultz@tpip.net> wrote: >> > GTP isn't special, I just don't like to have testing only features in there >> > when the same goal can be reached without having to add extra stuff. Adding >> > code that is not going to be useful in real production setups (or in this >> > case would even break production setups when enabled accidentally) makes the >> > implementation more complex than it needs to be. >> >> Well, you could make the same argument that allowing GTP to configured >> as standalone interface is a problem since GTP is only allowed to be >> with used with GTP-C. But, then we have something in the kernel that >> the community is expected to support, but requires jumping through a >> whole bunch of hoops just to run a simple netperf. > > "A whole bunch of hoops" without your new interface would consist of > running a single command-line program that is supplied with libgtpnl. > This is not a complete 3GPP network, but a simple libmnl-based helper > library with no other depenencies. > You have the point of view of someone who has a lot of experience dealing with this protocol. Try to imagine if you were some random kernel network programmer with no experience in the area. If they happen to find a one-off bug and want to do the right thing by running a test, you want to make that as easy as possible. From that perspective, building protocol specific libraries and finding the right cmd line to run is significant hoops (I can attest to this). There are other examples in the kernel of systems bigger than GTP that require a whole lot of effort just to run a simple test; you'll notice for those it's rare that best developers ever bother to look at them unless they're making a global change that affects the code. We don't want GTP to take be like that! > I'm not neccessarily against introducing features like the 'standalone > interface configuration'. However, we must make sure that any > significant new feature contributions like IPv6 are tested in a > "realistic setup" and not just using those 'interfaces added for easy > development'. Also, I would argue those 'interfaces added for easy > deveopment/benchmarking' should probably be clearly marked as such to > avoid raising the impression that this is what leads to a > standard-conforming / production-type setup. > Given the obvious complexity of running a real GTP stack, I don't think we have to worry about this. In order to test a "realistic setup" a whole bunch of other support is needed. So the forward looking question now is how to get to be able to run a "realistic setup"? Tom
Hi Tom, On Wed, Sep 20, 2017 at 05:55:01PM -0700, Tom Herbert wrote: > You have the point of view of someone who has a lot of experience > dealing with this protocol. Try to imagine if you were some random > kernel network programmer with no experience in the area. If they > happen to find a one-off bug and want to do the right thing by running > a test, you want to make that as easy as possible. Agreed. But we're not talking abut fixing a random bug in your patch series, but we're talking about adding significant new features - and those features need to be tested in real use caes, not just in an artificial test setup that holds assumptions that are not true. To improve performance, or to fix simple bugs that only affect the processing of the GTP-U G-PDU, a much more limited and hence "unrealistic" test scenario is probably sufficient/acceptable. > From that perspective, building protocol specific libraries and > finding the right cmd line to run is significant hoops (I can attest > to this). I understand your argument. But then, there is actually quite some tools to help you (see further below), as well as the wiki page at http://osmocom.org/projects/linux-kernel-gtp-u/wiki/Basic_Testing Of course, existing tools and existing wiki pages also only document existing features of the kernel code :) Yes, the documentation could be better. But then, how much more can you expect from somebody who's doing this mostly for fun and who - despite working in his dayjob on FOSS cellular projects - has no single commercial project/context that uses the kernel GTP code. In any case, working on a specific protocol or technology will require that you understand that technology to some extent, including the available tools. There's always a learning curve involved. > There are other examples in the kernel of systems bigger than GTP that > require a whole lot of effort just to run a simple test; you'll notice > for those it's rare that best developers ever bother to look at them > unless they're making a global change that affects the code. We don't > want GTP to take be like that! I'm all for following your argument. My point is simply: You cannot develop code solely based on mock-ups without any 'realistic' test scenarios. Otherwise you will end up with something that works only in your artificial lab setup, and follows all the best practises of the way how the Linux kernel traditionally approaches tunneling implementations, but it will never work/interop in the real world. And I'm very strongly opposed to merging code where we have not been able to show that it will inter-operate in at least one realistic scenario. This would raise wrong expectations with users and all sorts of downstream problems. So let's say we merge your IPv6 support as-is, and kernels get released + shipped with it. Later on, we find that in order to turn it into a standards-compliant implementation together with all the required bits in userspace and on the control plane, we need to change some parts of it, particularly those parts that affect the netlink or any other exposed userspace interface. At that point, we cannot change the interface as the kernel has a strict rule of never breaking userspace ABI. But we must change it in order to make it work in the real world. So what do we do? Add lots of cruft in order to emulate backwards compatibility? > So the forward looking question now is how to get to be able to run a > "realistic setup"? You can run this realistic setup entirely using Osmocom components. For running a 2G network: OsmoBTS+OsmoNITB+OsmoSGSN For running a 3G network: OsmoHNBGW+OsmoMSC+OsmoHLR+OsmoSGSN Both above stacks/combinations will provide you with GTP-C and GTP-U against a GGSN. As GGSN, you can then use either OpenGGSN, or OsmoGGSN, or ergw. For OpenGGSN and ergw, this will work with kernel GTP today, for those features present in kernel GTP (i.e. IPv4-only). In both cases you need some RF hardware. I'm happy to contribute related hardware (and support getting it set up) free of charge from my company sysmocom to anyone who has a realistic prospect of either * integrating your IPv6 support or other significant feature patches with libgtpnl + OsmoGGSN (at which point you can run a complete setup with real phones to verify it works end-to-end) * building and documenting or operating a continuous integration setup that would run tests on each new kernel version (or net-next, or whatever tree makes sense) to help us catch any regressions as the code proceeds In order to have a smaller, but still realistic test scenario, I implemented a series of GTP tests in http://git.osmocom.org/osmo-ttcn3-hacks/tree/ggsn_tests/GGSN_Tests.ttcn This code basically emulates the combination of everything from Phone to SGSN, so that you have only two entities: * the implementation under test = IUT (a GGSN implementing GTP-C + GTP-U) * the test itself (GGSN_Test) executing against the IUT The code so far implements PDP context activation + address allocation for IPv4-only and IPv6-only cases and can be run against a GGSN implementing those. The IPv6-only PDP context unit tests include the convoluted two-phase address assignment including sending the router solicitation from the simulated phone as well as verifying the router advertisement sent in response from the GGSN. Yes, I know they're written in an unknown niche programming language called TTCN-3, but this was the best tool at hand for the job I could find, and the tests are open source as is the Eclipse Titan toolchain for compiling it. We even have a Dockerfile that will build you a docker container containing the compiled GGSN_test at http://git.osmocom.org/docker-playground/tree/ggsn-test That docker container is used by a jenkins build test job to test current OsmoGGSN master every night against the test suite. I was stupid enough to break the testsuite with an accidential commit, so tests between August 27th and today failed. I've just reverted that accident - don't let that mistake of mine mislead you. I'm happy to contribute further to this by adding actual user-IP GTP-U functional testing beyond the router soliciatation/advertisement to it. So my suggestion in terms of a "realistic testbed without having to configure + run dozens of programs and using real RF + phones" is to use that testsuite. What one cannot get around is having to implement support for new features added on the kernel side such as IPv6 in libgtpnl and at least one of the GGSN's using it, sorry. Without that, there is no way to know if that code would do anything useful. You simply cannot realistically test GTP-U alone without GTP-C. I'd love to offer help on this, but it's really impossible right now. I'm on holidays on a motorbike tour through rural Taiwan's mountains, had to deal with a flat tire today, have limited connectivity and am already cutting down hard on sleep every night to be able to respond to the absolute minimally required work e-mails. And review/follow-up to your much appreciated patch series the last couple of days has also used a lot of (unexpected not scheduled for) time. I'm not complaining, I'm just saying I am really not able to contribute more to this effort right now beyond my review, the offer of free hardware for a real cellular network, and the extension of the test cases for GTP-U beyond the already implemented very important IPv6 address allocation/assignment which I believe your current code would not pass. Regards, Harald
On Thu, Sep 21, 2017 at 8:12 AM, Harald Welte <laforge@netfilter.org> wrote: > Hi Tom, > > On Wed, Sep 20, 2017 at 05:55:01PM -0700, Tom Herbert wrote: >> You have the point of view of someone who has a lot of experience >> dealing with this protocol. Try to imagine if you were some random >> kernel network programmer with no experience in the area. If they >> happen to find a one-off bug and want to do the right thing by running >> a test, you want to make that as easy as possible. > > Agreed. But we're not talking abut fixing a random bug in your patch > series, but we're talking about adding significant new features - and > those features need to be tested in real use caes, not just in an > artificial test setup that holds assumptions that are not true. > > To improve performance, or to fix simple bugs that only affect the > processing of the GTP-U G-PDU, a much more limited and hence > "unrealistic" test scenario is probably sufficient/acceptable. > >> From that perspective, building protocol specific libraries and >> finding the right cmd line to run is significant hoops (I can attest >> to this). > > I understand your argument. But then, there is actually quite some > tools to help you (see further below), as well as the wiki page at > http://osmocom.org/projects/linux-kernel-gtp-u/wiki/Basic_Testing > > Of course, existing tools and existing wiki pages also only document > existing features of the kernel code :) > > Yes, the documentation could be better. But then, how much more can you > expect from somebody who's doing this mostly for fun and who - despite > working in his dayjob on FOSS cellular projects - has no single > commercial project/context that uses the kernel GTP code. > > In any case, working on a specific protocol or technology will require > that you understand that technology to some extent, including the > available tools. There's always a learning curve involved. > >> There are other examples in the kernel of systems bigger than GTP that >> require a whole lot of effort just to run a simple test; you'll notice >> for those it's rare that best developers ever bother to look at them >> unless they're making a global change that affects the code. We don't >> want GTP to take be like that! > > I'm all for following your argument. My point is simply: You cannot > develop code solely based on mock-ups without any 'realistic' test > scenarios. Otherwise you will end up with something that works only in > your artificial lab setup, and follows all the best practises of the way > how the Linux kernel traditionally approaches tunneling implementations, > but it will never work/interop in the real world. > > And I'm very strongly opposed to merging code where we have not been > able to show that it will inter-operate in at least one realistic > scenario. This would raise wrong expectations with users and all sorts > of downstream problems. > Harald, Please see the cover letter for the original GTP kernel patches dated May 10, 2016. My first question on those was "Is there a timeline for adding IPv6 support?". To which Pablo replied that there was a preliminary patch for it that has not been released. That was almost a year and half ago and we have not heard anything since. If you don't like my patches or don't think that can be adapted to fully support the GTP specification, that's fine. But then you need to provide a viable alternative. We are at the point where a kernel networking feature that only supports IPv4 when it could support IPv6 must be considered incomplete. Thanks, Tom
Hi Tom, On Thu, Sep 21, 2017 at 09:43:02AM -0700, Tom Herbert wrote: > Please see the cover letter for the original GTP kernel patches dated > May 10, 2016. My first question on those was "Is there a timeline for > adding IPv6 support?". To which Pablo replied that there was a > preliminary patch for it that has not been released. I'll suggest Pablo to comment on that. I don't recall the details at that time, I was only involved in the earliest development of the module and then handed over. > If you don't like my patches or don't think that can be adapted to > fully support the GTP specification, that's fine. It's not about "not liking". I'm very happy about contributions, including (of course) yours. It's about making sure that code we merge into the kernel GTP driver will actually be usable to create a standards-compliant GTP application or not. There's no use in merging an IPv6 support patch if already by code review it can be shown that it's impossible to create a spec-compliant implementation using that patch. To me, that would be "merging IPv6 support so we can check off a box on a management form or marketing sheet", but not for any practical value. > But then you need to provide a viable alternative. Why do *I* have to provide a viable alternative? Who says that *I* have an obligation to do so? A (co-)maintainer of a given driver doesn't have the obligation of implementing any feature as requested. Community based collaborative development only gets those things done that people contribute. I have already contributed almost a decade of my life to creating Free Software implementations of cellular protocol stacks, and it continues to be the center of my work and spare time. GTP is only one protocol layer on one of those stacks. Pablo, Andreas and I have contributed a Linux kernel implementation that currently only implements IPv4. This implementation can by anyone extended to support IPv6, and as you see from this e-mail thread, there is interest in helping this along by * providing code review (even at times when it's personally difficult for me) * providing free hardware for setting up a "private cellular network" to test interoperability * providing testing tools for validation in absence of such a cellular network > We are at the point where a kernel networking feature that only > supports IPv4 when it could support IPv6 must be considered > incomplete. I agree it is incomplete. There's no doubt about that. But then, even the current "incomplete" implementation is working and can be used to operate an interoperable, spec-compatible IPv4 GGSN. So it serves a practical purpose. All I'm asking is that any IPv6 support patches are developed with that same practical purpose in mind. Going through the cover letter of your series again: > - IPv6 support Cannot be merged as-is, see lengthy review discussion > - Configurable networking interfaces so that GTP kernel can be > used and tested without needing GSN network emulation (i.e. no user > space daemon needed). > - Port numbers are configurable As I indicated, I'm not fundamentally opposed to it, but I'm wondering how much value they bring in reality. Andreas has raised the valid concern that we're adding code that is not used in production setups or by any of the userspace implementations using this tunneling module. The code gets more complex and gets code paths that will not be exercised/tested. Nevertheless, if it helps you to work on GTP, we can merge them from my point of view - unless Pablo and/or Andreas object more strongly. > - GSO,GRO > - A facility that allows application specific GSO callbacks Fine with me, but I think you need to convince other folks about the "application specific GSO" and the usage of the upper bits of shinfo(skb)->gso_type. > - Control of zero UDP checksums Same as above, Dave was raising some question about it, not sure if his concern remains. > - Addition of a dst_cache in the GTP structure Fine with me. As for the patches touching gtp.c: * 04/14 udp recv clean up: fine with me, but kbuild robot complaint? On a minor note, I think you're mixing two unrelated topics: Separating the UDP receive functions and conversion to gro_cells, which violates the "one patch per feature" rule. I'd still merge it, but would prefer two separate patches * 05/14 Remove special mtu handling Pending your rework * 06/14 Eliminate pktinfo and add port configuration I don't like the combination of a non-functional "cosmetic" refactoring of removing a data structure with the introduction of a new feature. Makes it harder to review, impossible to merge only one of the two. For the rationale of introducing the gtp_pktinfo struct, see http://git.osmocom.org/osmo-gtp-kernel/commit/?id=3bc7019c7afd06b5c7d94e5621728d092b82bb85 it was actually intended to make IPv6 support easier, but the partial IPv6 support was removed before mainline submission. * 07/14 Support encapsulation of IPv6 packets Not acceptable in its current form, see extensive review * 08/14 Support encpasulating over IPv6 No concerns in principle. Pending you making it dependent on AF of socket * 09/14 Allow configuring GTP interface as standalone Can be merged unless strong objection from Pablo/Andreas (see above) * 10/14 Add support for devnet No concerns from my side * 12/14 Configuration for zero UDP checksum Up to Dave, he raised a question on it * 13/14 Support for GRO No concerns from my side * 14/14 GSO support No concerns from my side BTW: Where have the iproute2/ip patches been posted, which you mention in your cover page of the patch series?
> It's not about "not liking". I'm very happy about contributions, > including (of course) yours. It's about making sure that code we merge > into the kernel GTP driver will actually be usable to create a > standards-compliant GTP application or not. > Harald, Do you believe that these patches are not at all on the right track, that they can't be built upon to get to a standards-compliant implementation, and that we are going to have to throw all of this and start from scratch to provide IPv6 support? > There's no use in merging an IPv6 support patch if already by code > review it can be shown that it's impossible to create a spec-compliant > implementation using that patch. To me, that would be "merging IPv6 > support so we can check off a box on a management form or marketing > sheet", but not for any practical value. > To be clear, these patches are not done because to be a bullet point on a marketing sheet. IPv6 is becoming _the_ Internet protocol. It continues to exhibit exponential growth (~20% of Internet, per Google stats), I believe least two of the largest datacenter operators are running everything over IPv6, and there are already proposals to start official deprecation of IPv4. In the mobile space IPv6 is going to be a critical enabler of IoT and security in technologies like 5G. If we want Linux to be at the forefront of the next technology wave then we need to focus on IPv6 now! We should be far past the days of vendors only providing IPv4 in the kernel support because "that's what our customers use" and they'll get to IPv6 support at their leisure. IMO, davem has every right to unilaterally NAK patches that only support IPv4 or only test IPv4 with not even a path or timeline for IPv6 support. Thanks, Tom
Hi Tom, On Sun, Sep 24, 2017 at 08:55:49AM -0700, Tom Herbert wrote: > Do you believe that these patches are not at all on the right track, > that they can't be built upon to get to a standards-compliant > implementation, and that we are going to have to throw all of this and > start from scratch to provide IPv6 support? I believe I have pointed out where the problem areas are, several times by now. I see no reason why things would have to be started from scratch. However, the issues pointed out in the IPv6 support patch[es] have to be resolved *before* any merge to mainline. I don't mind merging "incomplete" code that doesn't cover all parts of a spec but provides basic interoperability. I also am not arguing that code must be bug-free at the time it is merged (which is impossible anyway). But I am arguing that we cannot merge something that is a wrong implementation as per the spec, and hence it must be brought in-line with the spec before it can be merged. > > There's no use in merging an IPv6 support patch if already by code > > review it can be shown that it's impossible to create a spec-compliant > > implementation using that patch. To me, that would be "merging IPv6 > > support so we can check off a box on a management form or marketing > > sheet", but not for any practical value. > > To be clear, these patches are not done because to be a bullet point > on a marketing sheet. Great. > IPv6 is becoming _the_ Internet protocol. I'm all aware of that, and I've been a very early adopter, since the 1990ies with 6bone. My argument is not against IPv6 support. My argument is against merging something that introdues IPv6 in a way that's not in-line with the GTP protocol specifications, as such a way is of no use to anyone (except marketing sheets). > We should be far past the days of vendors only providing IPv4 in the > kernel support because "that's what our customers use" and they'll get > to IPv6 support at their leisure. I'm not sure where a "vendor" is involved with the GTP patches so far. I think we have to draw a distinction between what you expect from professional, corporate "vendors" with a commercial interest in mind (such as supporting their hardware) and what you can expect from people doing things in their spare time, out of enthusiasm to finally bring some Free Software into the closed world of telecommunications. The Telecom world should have implemented something like a GTP kernel module a decade to 15 years ago. They could have saved significant investments in proprietary hardware by running open source GGSNs with an accelerated user plane in the kernel. Nobody seemed to have an interest in that, until today - as you can see from Pablo and me working on this in our spare time, whenever we have a couple of spare cycles next to many other projects. You can see from the osmo-gtp-kernel commit log it took years of being a ultra-low-priority on-and-off project to ever get to a point where we thought it was worth submitting it mainline. Andreas deserves the praise for finally pushing it ahead. I'm looking forward to reviewing the next version of the patch series.
> I'm not sure where a "vendor" is involved with the GTP patches so far. I > think we have to draw a distinction between what you expect from > professional, corporate "vendors" with a commercial interest in mind > (such as supporting their hardware) and what you can expect from people > doing things in their spare time, out of enthusiasm to finally bring > some Free Software into the closed world of telecommunications. > If it makes you feel any better I am not getting paid for this work either :-) > The Telecom world should have implemented something like a GTP kernel > module a decade to 15 years ago. They could have saved significant > investments in proprietary hardware by running open source GGSNs with an > accelerated user plane in the kernel. Nobody seemed to have an interest > in that, until today - as you can see from Pablo and me working on this > in our spare time, whenever we have a couple of spare cycles next to > many other projects. You can see from the osmo-gtp-kernel commit log it > took years of being a ultra-low-priority on-and-off project to ever get > to a point where we thought it was worth submitting it mainline. > Andreas deserves the praise for finally pushing it ahead. > I completely agree, and your work is well appreciated! But I don't believe it is to late to steer the ship away from proprietary solutions. In fact, given the direction of the rest of the industry direction, now is our best opportunity to try. That is a major reason for these patches. We need to bring GTP into the limelight and get a lot more people thinking about. This might even be the world's most important tunneling protocol. If nothing else, a discussion like this is good if it inspires others in the community to start to look at it. Tom
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c index 121b41e7a901..1870469a4982 100644 --- a/drivers/net/gtp.c +++ b/drivers/net/gtp.c @@ -86,6 +86,9 @@ struct gtp_dev { struct sock *sk0; struct sock *sk1u; + struct socket *sock0; + struct socket *sock1u; + struct net_device *dev; unsigned int role; @@ -430,26 +433,33 @@ static void gtp_encap_destroy(struct sock *sk) } } -static void gtp_encap_disable_sock(struct sock *sk) +static void gtp_encap_release(struct gtp_dev *gtp) { - if (!sk) - return; + if (gtp->sk0) { + if (gtp->sock0) { + udp_tunnel_sock_release(gtp->sock0); + gtp->sock0 = NULL; + } else { + gtp_encap_destroy(gtp->sk0); + } - gtp_encap_destroy(sk); -} + gtp->sk0 = NULL; + } -static void gtp_encap_disable(struct gtp_dev *gtp) -{ - gtp_encap_disable_sock(gtp->sk0); - gtp_encap_disable_sock(gtp->sk1u); + if (gtp->sk1u) { + if (gtp->sock1u) { + udp_tunnel_sock_release(gtp->sock1u); + gtp->sock1u = NULL; + } else { + gtp_encap_destroy(gtp->sk1u); + } + + gtp->sk1u = NULL; + } } static int gtp_dev_init(struct net_device *dev) { - struct gtp_dev *gtp = netdev_priv(dev); - - gtp->dev = dev; - dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats); if (!dev->tstats) return -ENOMEM; @@ -461,7 +471,8 @@ static void gtp_dev_uninit(struct net_device *dev) { struct gtp_dev *gtp = netdev_priv(dev); - gtp_encap_disable(gtp); + gtp_encap_release(gtp); + free_percpu(dev->tstats); } @@ -676,6 +687,7 @@ static const struct net_device_ops gtp_netdev_ops = { static void gtp_link_setup(struct net_device *dev) { + struct gtp_dev *gtp = netdev_priv(dev); dev->netdev_ops = >p_netdev_ops; dev->needs_free_netdev = true; @@ -697,6 +709,8 @@ static void gtp_link_setup(struct net_device *dev) sizeof(struct udphdr) + sizeof(struct gtp0_header); + gtp->dev = dev; + gro_cells_init(>p->gro_cells, dev); } @@ -710,13 +724,19 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev, struct netlink_ext_ack *extack) { unsigned int role = GTP_ROLE_GGSN; + bool have_fd, have_ports; bool is_ipv6 = false; struct gtp_dev *gtp; struct gtp_net *gn; int hashsize, err; - if (!data[IFLA_GTP_FD0] && !data[IFLA_GTP_FD1]) + have_fd = !!data[IFLA_GTP_FD0] || !!data[IFLA_GTP_FD1]; + have_ports = !!data[IFLA_GTP_PORT0] || !!data[IFLA_GTP_PORT1]; + + if (!(have_fd ^ have_ports)) { + /* Either got fd(s) or port(s) */ return -EINVAL; + } if (data[IFLA_GTP_ROLE]) { role = nla_get_u32(data[IFLA_GTP_ROLE]); @@ -773,7 +793,7 @@ static int gtp_newlink(struct net *src_net, struct net_device *dev, out_hashtable: gtp_hashtable_free(gtp); out_encap: - gtp_encap_disable(gtp); + gtp_encap_release(gtp); return err; } @@ -782,7 +802,7 @@ static void gtp_dellink(struct net_device *dev, struct list_head *head) struct gtp_dev *gtp = netdev_priv(dev); gro_cells_destroy(>p->gro_cells); - gtp_encap_disable(gtp); + gtp_encap_release(gtp); gtp_hashtable_free(gtp); list_del_rcu(>p->list); unregister_netdevice_queue(dev, head); @@ -793,6 +813,8 @@ static const struct nla_policy gtp_policy[IFLA_GTP_MAX + 1] = { [IFLA_GTP_FD1] = { .type = NLA_U32 }, [IFLA_GTP_PDP_HASHSIZE] = { .type = NLA_U32 }, [IFLA_GTP_ROLE] = { .type = NLA_U32 }, + [IFLA_GTP_PORT0] = { .type = NLA_U16 }, + [IFLA_GTP_PORT1] = { .type = NLA_U16 }, }; static int gtp_validate(struct nlattr *tb[], struct nlattr *data[], @@ -883,11 +905,35 @@ static void gtp_hashtable_free(struct gtp_dev *gtp) kfree(gtp->tid_hash); } -static struct sock *gtp_encap_enable_socket(int fd, int type, - struct gtp_dev *gtp, - bool is_ipv6) +static int gtp_encap_enable_sock(struct socket *sock, int type, + struct gtp_dev *gtp) { struct udp_tunnel_sock_cfg tuncfg = {NULL}; + + switch (type) { + case UDP_ENCAP_GTP0: + tuncfg.encap_rcv = gtp0_udp_encap_recv; + break; + case UDP_ENCAP_GTP1U: + tuncfg.encap_rcv = gtp1u_udp_encap_recv; + break; + default: + pr_debug("Unknown encap type %u\n", type); + return -EINVAL; + } + + tuncfg.sk_user_data = gtp; + tuncfg.encap_type = type; + tuncfg.encap_destroy = gtp_encap_destroy; + + setup_udp_tunnel_sock(sock_net(sock->sk), sock, &tuncfg); + + return 0; +} + +static struct sock *gtp_encap_enable_fd(int fd, int type, struct gtp_dev *gtp, + bool is_ipv6) +{ struct socket *sock; struct sock *sk; int err; @@ -920,60 +966,124 @@ static struct sock *gtp_encap_enable_socket(int fd, int type, sk = sock->sk; sock_hold(sk); - switch (type) { - case UDP_ENCAP_GTP0: - tuncfg.encap_rcv = gtp0_udp_encap_recv; - break; - case UDP_ENCAP_GTP1U: - tuncfg.encap_rcv = gtp1u_udp_encap_recv; - break; - default: - pr_debug("Unknown encap type %u\n", type); - sk = ERR_PTR(-EINVAL); - goto out_sock; - } - - tuncfg.sk_user_data = gtp; - tuncfg.encap_type = type; - tuncfg.encap_destroy = gtp_encap_destroy; - - setup_udp_tunnel_sock(sock_net(sock->sk), sock, &tuncfg); + err = gtp_encap_enable_sock(sock, type, gtp); + if (err < 0) + sk = ERR_PTR(err); out_sock: sockfd_put(sock); return sk; } +static struct socket *gtp_create_sock(struct net *net, bool ipv6, + __be16 port, u32 flags) +{ + struct socket *sock; + struct udp_port_cfg udp_conf; + int err; + + memset(&udp_conf, 0, sizeof(udp_conf)); + + if (ipv6) { + udp_conf.family = AF_INET6; + udp_conf.ipv6_v6only = 1; + } else { + udp_conf.family = AF_INET; + } + + udp_conf.local_udp_port = port; + + /* Open UDP socket */ + err = udp_sock_create(net, &udp_conf, &sock); + if (err) + return ERR_PTR(err); + + return sock; +} + static int gtp_encap_enable(struct gtp_dev *gtp, struct nlattr *data[], bool is_ipv6) { + int err; + + struct socket *sock0 = NULL, *sock1u = NULL; struct sock *sk0 = NULL, *sk1u = NULL; if (data[IFLA_GTP_FD0]) { u32 fd0 = nla_get_u32(data[IFLA_GTP_FD0]); - sk0 = gtp_encap_enable_socket(fd0, UDP_ENCAP_GTP0, gtp, - is_ipv6); - if (IS_ERR(sk0)) - return PTR_ERR(sk0); + sk0 = gtp_encap_enable_fd(fd0, UDP_ENCAP_GTP0, gtp, is_ipv6); + if (IS_ERR(sk0)) { + err = PTR_ERR(sk0); + sk0 = NULL; + goto out_err; + } + } else if (data[IFLA_GTP_PORT0]) { + __be16 port = nla_get_u16(data[IFLA_GTP_PORT0]); + + sock0 = gtp_create_sock(dev_net(gtp->dev), is_ipv6, port, 0); + if (IS_ERR(sock0)) { + err = PTR_ERR(sock0); + sock0 = NULL; + goto out_err; + } + + err = gtp_encap_enable_sock(sock0, UDP_ENCAP_GTP0, gtp); + if (err) + goto out_err; } if (data[IFLA_GTP_FD1]) { u32 fd1 = nla_get_u32(data[IFLA_GTP_FD1]); - sk1u = gtp_encap_enable_socket(fd1, UDP_ENCAP_GTP1U, gtp, - is_ipv6); + sk1u = gtp_encap_enable_fd(fd1, UDP_ENCAP_GTP1U, gtp, is_ipv6); if (IS_ERR(sk1u)) { - if (sk0) - gtp_encap_disable_sock(sk0); - return PTR_ERR(sk1u); + err = PTR_ERR(sk1u); + sk1u = NULL; + goto out_err; + } + } else if (data[IFLA_GTP_PORT1]) { + __be16 port = nla_get_u16(data[IFLA_GTP_PORT1]); + + sock1u = gtp_create_sock(dev_net(gtp->dev), is_ipv6, port, 0); + if (IS_ERR(sock1u)) { + err = PTR_ERR(sock1u); + sock1u = NULL; + goto out_err; } + + err = gtp_encap_enable_sock(sock1u, UDP_ENCAP_GTP1U, gtp); + if (err) + goto out_err; + } + + if (sock0) { + gtp->sock0 = sock0; + gtp->sk0 = sock0->sk; + } else { + gtp->sk0 = sk0; } - gtp->sk0 = sk0; - gtp->sk1u = sk1u; + if (sock1u) { + gtp->sock1u = sock1u; + gtp->sk1u = sock1u->sk; + } else { + gtp->sk1u = sk1u; + } return 0; + +out_err: + if (sk0) + gtp_encap_destroy(sk0); + if (sk1u) + gtp_encap_destroy(sk1u); + if (sock0) + udp_tunnel_sock_release(sock0); + if (sock1u) + udp_tunnel_sock_release(sock1u); + + return err; } static struct gtp_dev *gtp_find_dev(struct net *src_net, struct nlattr *nla[]) @@ -1515,8 +1625,8 @@ static const struct genl_ops gtp_genl_ops[] = { }; static struct genl_family gtp_genl_family __ro_after_init = { - .name = "gtp", - .version = 0, + .name = GTP_GENL_NAME, + .version = GTP_GENL_VERSION, .hdrsize = 0, .maxattr = GTPA_MAX, .netnsok = true, diff --git a/include/uapi/linux/gtp.h b/include/uapi/linux/gtp.h index 8eec519fa754..0da18aa88be8 100644 --- a/include/uapi/linux/gtp.h +++ b/include/uapi/linux/gtp.h @@ -9,6 +9,11 @@ enum gtp_genl_cmds { GTP_CMD_MAX, }; +/* NETLINK_GENERIC related info + */ +#define GTP_GENL_NAME "gtp" +#define GTP_GENL_VERSION 0 + enum gtp_version { GTP_V0 = 0, GTP_V1,
Add new configuration of GTP interfaces that allow specifying a port to listen on (as opposed to having to get sockets from a userspace control plane). This allows GTP interfaces to be configured and the data path tested without requiring a GTP-C daemon. Signed-off-by: Tom Herbert <tom@quantonium.net> --- drivers/net/gtp.c | 212 +++++++++++++++++++++++++++++++++++------------ include/uapi/linux/gtp.h | 5 ++ 2 files changed, 166 insertions(+), 51 deletions(-)