Re: [PATCH net-next 11/15] sctp: add udphdr to overhead when udp_port is set

From: Michael Tuexen <tuexen@fh-muenster.de>
To: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Cc: Xin Long <lucien.xin@gmail.com>,
	kernel test robot <lkp@intel.com>,
	network dev <netdev@vger.kernel.org>,
	linux-sctp@vger.kernel.org, kbuild-all@lists.01.org,
	Neil Horman <nhorman@tuxdriver.com>, davem <davem@davemloft.net>
Subject: Re: [PATCH net-next 11/15] sctp: add udphdr to overhead when udp_port is set
Date: Mon, 05 Oct 2020 20:08:36 +0000	[thread overview]
Message-ID: <029F42A9-31CF-45B6-944D-912A005EE864@fh-muenster.de> (raw)
In-Reply-To: <20201005190114.GL70998@localhost.localdomain>

[-- Attachment #1: Type: text/plain, Size: 4647 bytes --]

> On 5. Oct 2020, at 21:01, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> 
> On Sat, Oct 03, 2020 at 08:24:34PM +0800, Xin Long wrote:
>> On Sat, Oct 3, 2020 at 7:23 PM Xin Long <lucien.xin@gmail.com> wrote:
>>> 
>>> On Sat, Oct 3, 2020 at 4:12 PM Xin Long <lucien.xin@gmail.com> wrote:
>>>> 
>>>> On Sat, Oct 3, 2020 at 12:08 PM Marcelo Ricardo Leitner
>>>> <marcelo.leitner@gmail.com> wrote:
>>>>> 
>>>>> On Wed, Sep 30, 2020 at 03:00:42AM +0800, kernel test robot wrote:
>>>>>> Hi Xin,
>>>>>> 
>>>>>> Thank you for the patch! Yet something to improve:
>>>>> 
>>>>> I wonder how are you planning to fix this. It is quite entangled.
>>>>> This is not performance critical. Maybe the cleanest way out is to
>>>>> move it to a .c file.
>>>>> 
>>>>> Adding a
>>>>> #if defined(CONFIG_IP_SCTP) || defined(CONFIG_IP_SCTP_MODULE)
>>>>> in there doesn't seem good.
>>>>> 
>>>>>>   In file included from include/net/sctp/checksum.h:27,
>>>>>>                    from net/netfilter/nf_nat_proto.c:16:
>>>>>>   include/net/sctp/sctp.h: In function 'sctp_mtu_payload':
>>>>>>>> include/net/sctp/sctp.h:583:31: error: 'struct net' has no member named 'sctp'; did you mean 'ct'?
>>>>>>     583 |   if (sock_net(&sp->inet.sk)->sctp.udp_port)
>>>>>>         |                               ^~~~
>>>>>>         |                               ct
>>>>>> 
>>>> Here is actually another problem, I'm still thinking how to fix it.
>>>> 
>>>> Now sctp_mtu_payload() returns different value depending on
>>>> net->sctp.udp_port. but net->sctp.udp_port can be changed by
>>>> "sysctl -w" anytime. so:
> 
> Good point.
> 
>>>> 
>>>> In sctp_packet_config() it gets overhead/headroom by calling
>>>> sctp_mtu_payload(). When 'udp_port' is 0, it's IP+MAC header
>>>> size. Then if 'udp_port' is changed to 9899 by 'sysctl -w',
>>>> udphdr will also be added to the packet in sctp_v4_xmit(),
>>>> and later the headroom may not be enough for IP+MAC headers.
>>>> 
>>>> I'm thinking to add sctp_sock->udp_port, and it'll be set when
>>>> the sock is created with net->udp_port. but not sure if we should
>>>> update sctp_sock->udp_port with  net->udp_port when sending packets?
> 
> I don't think so,
> 
>>> something like:
> ...
>> diff --git a/net/sctp/output.c b/net/sctp/output.c
>> index 6614c9fdc51e..c96b13ec72f4 100644
>> --- a/net/sctp/output.c
>> +++ b/net/sctp/output.c
>> @@ -91,6 +91,14 @@ void sctp_packet_config(struct sctp_packet *packet,
>> __u32 vtag,
>>        if (asoc) {
>>                sk = asoc->base.sk;
>>                sp = sctp_sk(sk);
>> +
>> +               if (unlikely(sp->udp_port != sock_net(sk)->sctp.udp_port)) {
> 
> RFC6951 has:
> 
> 6.1.  Get or Set the Remote UDP Encapsulation Port Number
>      (SCTP_REMOTE_UDP_ENCAPS_PORT)
> ...
>   sue_assoc_id:  This parameter is ignored for one-to-one style
>      sockets.  For one-to-many style sockets, the application may fill
>      in an association identifier or SCTP_FUTURE_ASSOC for this query.
>      It is an error to use SCTP_{CURRENT|ALL}_ASSOC in sue_assoc_id.
> 
>   sue_address:  This specifies which address is of interest.  If a
>      wildcard address is provided, it applies only to future paths.
> 
> So I'm not seeing a reason to have a system wide knob that takes
> effect in run time like this.
> Enable, start apps, and they keep behaving as initially configured.
> Need to disable? Restart the apps/sockets.
> 
> Thoughts?
Just note about how things are implemented in FreeBSD. This is not about
how it has to be implemented, but how it can be implemented.

The local UDP encaps port is controlled by a system wide sysctl.
sudo sysctl net.inet.sctp.udp_tunneling_port=9899
will use from that point on port 9899 the local encaps port. The
local encaps port can't be controlled by the application.
sudo sysctl net.inet.sctp.udp_tunneling_port=9900
will change this port number instantly to 9900. I don't see a
use case for this, but it is possible.
sudo sysctl net.inet.sctp.udp_tunneling_port=0
disables the sending and receiving of UDP encapsulated packets.

The application can only control the remote encapsulation port on
a per remote address base. This is mostly relevant for the client
side wanting to use UDP encapsulation.

Best regards
Michael
> 
>> +                       __u16 port = sock_net(sk)->sctp.udp_port;
>> +
>> +                       if (!sp->udp_port || !port)
>> +                               sctp_assoc_update_frag_point(asoc);
>> +                       sp->udp_port = port;
>> +               }
>>        }

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5257 bytes --]