Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Nov 2022 14:29:38 +0800
From:   Firo Yang <firo.yang@suse.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Cc:     vyasevich@gmail.com, nhorman@tuxdriver.com, mkubecek@suse.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, linux-sctp@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        firogm@gmail.com
Subject: Re: [PATCH 1/1] sctp: sysctl: referring the correct net namespace
Message-ID: <Y38PUmjeFWApHnrh@suse.com>
References: <20221123094406.32654-1-firo.yang@suse.com>
 <Y34ZVEeSryB0UTFD@t14s.localdomain>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Y34ZVEeSryB0UTFD@t14s.localdomain>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bjFEcU40bXJkQzFTNE1KVU9sMWpJNDk2NzNVOFFqdlVXZE1RdmdhVFpLMU1q?=
 =?utf-8?B?UTE3L0pzSWdVYnRDOTdsbStnTTB6WmcwOVR2UzFBcU1yVlVtVmx5UXNrZDQr?=
 =?utf-8?B?SC9oNmE4cUlQOWx0SjVhRmFNd2VEcEZhaXh5RjVaMk5aYjZNcTJNeW0zRkhB?=
 =?utf-8?B?THYxN203WmxBOFVBbXdDaU1uNGxLVE5iK3o1amhRTHExTXBSOTRVei9XMUFM?=
 =?utf-8?B?akNBQjJZQnpzTWlmK2UrclhBTHgyZEdDUDNEbVVPeWh0Y0cvQUFDM3c4eUQ2?=
 =?utf-8?B?OXFTR1FPaGpqRHA4VG9aVVpqNDVZSXphQStQTTJ0TnQ3VWRPNW9XMHMxVWZh?=
 =?utf-8?B?UUdzemZ3NThHcVE0WEpGT1ZZQ1FKUDNKaEpqWWkyUDdzSTJVREVBSXdjZGZr?=
 =?utf-8?B?cjhBbk5FL0MvZVo0eEZYODF6aHlIZVhaMng1bENHRGgxY1orQnJJK1RqNTNs?=
 =?utf-8?B?Y3kxKzVwRDFmdnpXRFQwdXlqRXlCSXVkdFIvSzAzZThQZWRYRW05aFhXTUM2?=
 =?utf-8?B?YjEyRktmZnhCNXgxaTVZcjFtZzJaeTk3bjc2YUJsdDh3cGFzVXFnOGNKUi9U?=
 =?utf-8?B?NVpDSVV6Q08xZlpQcDdWak9wc014WTVPeEd1Z3pvdnFzNFlQeGE1KzEyWGIx?=
 =?utf-8?B?SDRnVXRrcXlUSHlsQjk0TlpNcGRER3U5U0NGd2laTk5WTmxlVE1PTWRhQ1E2?=
 =?utf-8?B?VmUrU1E3OEhOODhSZnRRaVVMcFcvcVpIVm8zVHIwcDhpUVdWTUFKZjViTENJ?=
 =?utf-8?B?djBPTGt6VFVnZHNtR2podS9DVGxkYlNYdkZZNWdmamdjWHlwaGEzS2FxQ01O?=
 =?utf-8?B?dE5Ld2VpSVEvbFROZm5GN2RhN0RKM29BSDFiN0VFN2MyajlsLyt4bE9wM1FJ?=
 =?utf-8?B?TzAvL0dEY2hLVmJBZ2hrR3IyRG1wY0MzL2RsUHpsUnNXUnpMcTJFNTNlR3Qv?=
 =?utf-8?B?ZnBzQUxwZXBHQjdHRHZVbElJQW50d1cwTFF1b1cvVnlmV29nQVlrZXZVRXR6?=
 =?utf-8?B?TDVHNFp4RkUvdHA2eTgrUlJWWFd2T2FkZmppcCtIM0h0RVZUa2RPY2RwMFhT?=
 =?utf-8?B?NUdJbXRGdDhtK2tMSnZ5S0ZYVFIrRi90ODJNUHg1YUM4Rm5mVVhndXBqRXhh?=
 =?utf-8?B?L2ZZelpPR2w1MUNnSnJheWMxTDFMMCt3bmZVMEFkakZGREFTQmc4MmFPejhR?=
 =?utf-8?B?ZHBsMGF5RmdBalN6cnZ3ZTZOQWd0MmFlaTRDT1pkMjZwWko2eGQvREdFb3lU?=
 =?utf-8?B?WWNtMVc0NktJZjFPNmN1Q3RUb1pnc0JzY0hocGdKT1liNDNYaExpYW9uLzE5?=
 =?utf-8?B?YXBMNzByeHk0NlZqakpiZXZYNW0wb3Z5T1ptWDd1VkJMM1BHUzFyd2l2RlVP?=
 =?utf-8?B?Rms5Yng1TUVveHVJMkhZSXFMejVyWXl5aFl4WWd4TitWZzZtN0N2dmd6N29C?=
 =?utf-8?B?bXVHVDRSTCtiQk5JanpPaWhmQ3FBSndzWWdPRGlOa2x1WlpHd2xJT0JpNDZ3?=
 =?utf-8?B?SXI5V2JtYms5UkJ0RVpocDFIL2tIRDNEcWp5R2R4QjNoMWtoWHgwZHpPR1h6?=
 =?utf-8?B?OTAzODVsUkVlbDh6OUJwMUMyNWUzTGM3dXllV29RZlZRczN0VTlFWVE3V2c1?=
 =?utf-8?B?YlFPYnl5UDBkTEpTd2F1QnVRRjNBWDViUWdUa0hsQ3Z0blB5bjdQdWU1MzM2?=
 =?utf-8?B?Uis1S2RCUnpWd3hZTnBnaE1zRHBINkFua3g0SHV4WjgzaDVXbGw4T3ZTWU53?=
 =?utf-8?B?YjFqU014ZzhoRHlZQTRJTjQ1QW9WQ2lhVC8vRGpLYTNPeGl4bHhOUXM1djg3?=
 =?utf-8?B?M2taM2REVFU5REc2akpVYTB1WlJKS1Z2allTMmN4b2xUTk4yWGUydzdnODdK?=
 =?utf-8?B?RVVUS2ozLzMrNnQwanpvemhPQ2dXQ1AvSkxSQjVOWEVubUUxNVBPbzRFSDlx?=
 =?utf-8?B?M2tvL0RTYllabG53NzhWdnRaTWNrVURoOWV1emRvbkdLQnpqNVRGdGxyZGxJ?=
 =?utf-8?B?RHlWaHlGNUFHVTRiZmZKdGI5NG1raVljR05SVWVRaXF4WmhjeDNFNWozbVlU?=
 =?utf-8?B?NDVHUFJPVWUvY3BqOUNJamdHOGVpRFltWmJFcDd0ajlvTW9yOGlWakF5eTJW?=
 =?utf-8?B?UlJDN2djWG9YMU8wRzJ1OXVJT1lHNjExOGFpNzhnSzZDTWROdkxWMnRWN3BH?=
 =?utf-8?Q?Pc6qqLjCDGqDNEMZHCblkxN4fexq7PUq8UTEWSBYG0wh?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a846dca4-e81c-47c5-4cea-08dacde54ec8
X-MS-Exchange-CrossTenant-AuthSource: AS8PR04MB9009.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Nov 2022 06:29:58.3831
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 6RRJ1N7BFotvFTSUktsAm2RnJVhGw29/f1VpIXQojMnshe4QuDK5j7VPgz7Qq3oosBmHUIHgPAPXA5L7OCAV2A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB8561
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

The 11/23/2022 10:00, Marcelo Ricardo Leitner wrote:
> On Wed, Nov 23, 2022 at 05:44:06PM +0800, Firo Yang wrote:
> > Recently, a customer reported that from their container whose
> > net namespace is different to the host's init_net, they can't set
> > the container's net.sctp.rto_max to any value smaller than
> > init_net.sctp.rto_min.
> > 
> > For instance,
> > Host:
> > sudo sysctl net.sctp.rto_min
> > net.sctp.rto_min = 1000
> > 
> > Container:
> > echo 100 > /mnt/proc-net/sctp/rto_min
> > echo 400 > /mnt/proc-net/sctp/rto_max
> > echo: write error: Invalid argument
> > 
> > This is caused by the check made from this'commit 4f3fdf3bc59c
> > ("sctp: add check rto_min and rto_max in sysctl")'
> > When validating the input value, it's always referring the boundary
> > value set for the init_net namespace.
> > 
> > Having container's rto_max smaller than host's init_net.sctp.rto_min
> > does make sense. Considering that the rto between two containers on the
> > same host is very likely smaller than it for two hosts.
> 
> Makes sense. And also, here, it is not using the init_net as
> boundaries for the values themselves. I mean, rto_min in init_net
> won't be the minimum allowed for rto_min in other netns. Ditto for
> rto_max.
> 
> More below.
> 
> > 
> > So to fix this problem, just referring the boundary value from the net
> > namespace where the new input value came from shold be enough.
> > 
> > Signed-off-by: Firo Yang <firo.yang@suse.com>
> > ---
> >  net/sctp/sysctl.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> > index b46a416787ec..e167df4dc60b 100644
> > --- a/net/sctp/sysctl.c
> > +++ b/net/sctp/sysctl.c
> > @@ -429,6 +429,9 @@ static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
> >  	else
> >  		tbl.data = &net->sctp.rto_min;
> >  
> > +	if (net != &init_net)
> > +		max = net->sctp.rto_max;
> 
> This also affects other sysctls:
> 
> $ grep -e procname -e extra sysctl.c | grep -B1 extra.*init_net
>                 .extra1         = SYSCTL_ONE,
>                 .extra2         = &init_net.sctp.rto_max
>                 .procname       = "rto_max",
>                 .extra1         = &init_net.sctp.rto_min,
> --
>                 .extra1         = SYSCTL_ZERO,
>                 .extra2         = &init_net.sctp.ps_retrans,
>                 .procname       = "ps_retrans",
>                 .extra1         = &init_net.sctp.pf_retrans,
> 
> And apparently, SCTP is the only one doing such dynamic limits. At
> least in networking.
> 
> While the issue you reported is fixable this way, for ps/pf_retrans,
> it is not, as it is using proc_dointvec_minmax() and it will simply
> consume those values (with no netns translation).
> 
> So what about patching sctp_sysctl_net_register() instead, to update
> these pointers during netns creation? Right after where it update the
> 'data' one in there:
> 
>         for (i = 0; table[i].data; i++)
>                 table[i].data += (char *)(&net->sctp) - (char *)&init_net.sctp;

Thanks Marcelo. It's better. So you mean something like the following?

--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -586,6 +586,11 @@ int sctp_sysctl_net_register(struct net *net)
        for (i = 0; table[i].data; i++)
                table[i].data += (char *)(&net->sctp) - (char *)&init_net.sctp;
 
+#define SCTP_RTO_MIN_IDX 1
+#define SCTP_RTO_MAX_IDX 2
+       table[SCTP_RTO_MIN_IDX].extra2 = &net->sctp.rto_max;
+       table[SCTP_RTO_MAX_IDX].extra1 = &net->sctp.rto_min;
+
        net->sctp.sysctl_header = register_net_sysctl(net, "net/sctp", table);
        if (net->sctp.sysctl_header == NULL) {
                kfree(table);


> 
> Thanks,
> Marcelo
> 
> > +
> >  	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
> >  	if (write && ret == 0) {
> >  		if (new_value > max || new_value < min)
> > @@ -457,6 +460,9 @@ static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
> >  	else
> >  		tbl.data = &net->sctp.rto_max;
> >  
> > +	if (net != &init_net)
> > +		min = net->sctp.rto_min;
> > +
> >  	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
> >  	if (write && ret == 0) {
> >  		if (new_value > max || new_value < min)
> > -- 
> > 2.26.2
> >