From: Neil Horman <nhorman@tuxdriver.com> To: Wang Weidong <wangweidong1@huawei.com> Cc: David Miller <davem@davemloft.net>, Vlad Yasevich <vyasevich@gmail.com>, Daniel Borkmann <dborkman@redhat.com>, linux-sctp@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [PATCH net-next] sctp: add a checking for sctp_sysctl_net_register Date: Thu, 8 May 2014 07:41:24 -0400 [thread overview] Message-ID: <20140508114124.GD9696@hmsreliant.think-freely.org> (raw) In-Reply-To: <536B6AFD.40303@huawei.com> On Thu, May 08, 2014 at 07:31:09PM +0800, Wang Weidong wrote: > On 2014/5/8 19:26, Wang Weidong wrote: > > On 2014/5/8 19:11, Neil Horman wrote: > >> On Thu, May 08, 2014 at 03:55:05PM +0800, Wang Weidong wrote: > >>> When register_net_sysctl failed, we should free the > >>> sysctl_table while the net_namespace is not init_net. > >>> > >>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com> > >>> --- > >>> net/sctp/sysctl.c | 5 +++++ > >>> 1 file changed, 5 insertions(+) > >>> > >>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c > >>> index 844d2b0..4c17694 100644 > >>> --- a/net/sctp/sysctl.c > >>> +++ b/net/sctp/sysctl.c > >>> @@ -450,6 +450,11 @@ int sctp_sysctl_net_register(struct net *net) > >>> } > >>> > >>> net->sctp.sysctl_header = register_net_sysctl(net, "net/sctp", table); > >>> + if (net->sctp.sysctl_header == NULL) { > >>> + if (!net_eq(net, &init_net)) > >>> + kfree(table); > >>> + return -ENOMEM; > >>> + } > >> Sorry, we're not special casing this in 3 places. Why not just revert the > >> origonal commit so that all the sysctl tables can be handled the same way > >> Neil > >> > > Hi Neil, > > > > The original commit avoids to kmemdup > > > Hi Neil, > > Sorry for the accomplished mail. > Although, revert the original commit, the checking for net->sctp.sysctl_header > is need from coding view. > > What do you think? > That was my review. I get that the origional commit avoids kmemdup for the init_net case. My point was that, in so doing you created the need to special case the handling of the sysctl table for init_net in 3 places now. It seems to me that, while its always nice to save space, it would be cleaner programming to just accept the need for the extra 1.6Kb of space and not have to worry about the special casing. Neil > Regards > Wang > > >>> return 0; > >>> } > >>> > >>> -- > >>> 1.7.12 > >>> > >>> > >>> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe netdev" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > >> > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
WARNING: multiple messages have this Message-ID (diff)
From: Neil Horman <nhorman@tuxdriver.com> To: Wang Weidong <wangweidong1@huawei.com> Cc: David Miller <davem@davemloft.net>, Vlad Yasevich <vyasevich@gmail.com>, Daniel Borkmann <dborkman@redhat.com>, linux-sctp@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [PATCH net-next] sctp: add a checking for sctp_sysctl_net_register Date: Thu, 08 May 2014 11:41:24 +0000 [thread overview] Message-ID: <20140508114124.GD9696@hmsreliant.think-freely.org> (raw) In-Reply-To: <536B6AFD.40303@huawei.com> On Thu, May 08, 2014 at 07:31:09PM +0800, Wang Weidong wrote: > On 2014/5/8 19:26, Wang Weidong wrote: > > On 2014/5/8 19:11, Neil Horman wrote: > >> On Thu, May 08, 2014 at 03:55:05PM +0800, Wang Weidong wrote: > >>> When register_net_sysctl failed, we should free the > >>> sysctl_table while the net_namespace is not init_net. > >>> > >>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com> > >>> --- > >>> net/sctp/sysctl.c | 5 +++++ > >>> 1 file changed, 5 insertions(+) > >>> > >>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c > >>> index 844d2b0..4c17694 100644 > >>> --- a/net/sctp/sysctl.c > >>> +++ b/net/sctp/sysctl.c > >>> @@ -450,6 +450,11 @@ int sctp_sysctl_net_register(struct net *net) > >>> } > >>> > >>> net->sctp.sysctl_header = register_net_sysctl(net, "net/sctp", table); > >>> + if (net->sctp.sysctl_header = NULL) { > >>> + if (!net_eq(net, &init_net)) > >>> + kfree(table); > >>> + return -ENOMEM; > >>> + } > >> Sorry, we're not special casing this in 3 places. Why not just revert the > >> origonal commit so that all the sysctl tables can be handled the same way > >> Neil > >> > > Hi Neil, > > > > The original commit avoids to kmemdup > > > Hi Neil, > > Sorry for the accomplished mail. > Although, revert the original commit, the checking for net->sctp.sysctl_header > is need from coding view. > > What do you think? > That was my review. I get that the origional commit avoids kmemdup for the init_net case. My point was that, in so doing you created the need to special case the handling of the sysctl table for init_net in 3 places now. It seems to me that, while its always nice to save space, it would be cleaner programming to just accept the need for the extra 1.6Kb of space and not have to worry about the special casing. Neil > Regards > Wang > > >>> return 0; > >>> } > >>> > >>> -- > >>> 1.7.12 > >>> > >>> > >>> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe netdev" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > >> > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
next prev parent reply other threads:[~2014-05-08 11:41 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-05-08 7:55 [PATCH net-next] sctp: add a checking for sctp_sysctl_net_register Wang Weidong 2014-05-08 7:55 ` Wang Weidong 2014-05-08 9:40 ` tingwei liu 2014-05-08 9:40 ` tingwei liu 2014-05-08 11:11 ` Neil Horman 2014-05-08 11:11 ` Neil Horman 2014-05-08 11:26 ` Wang Weidong 2014-05-08 11:26 ` Wang Weidong 2014-05-08 11:31 ` Wang Weidong 2014-05-08 11:31 ` Wang Weidong 2014-05-08 11:41 ` Neil Horman [this message] 2014-05-08 11:41 ` Neil Horman 2014-05-08 11:57 ` Wang Weidong 2014-05-08 11:57 ` Wang Weidong
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=20140508114124.GD9696@hmsreliant.think-freely.org \ --to=nhorman@tuxdriver.com \ --cc=davem@davemloft.net \ --cc=dborkman@redhat.com \ --cc=linux-sctp@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=vyasevich@gmail.com \ --cc=wangweidong1@huawei.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.