* [PATCH net-next 1/3] rtnl: allocate more attr tables on the heap
2022-04-29 23:55 [PATCH net-next 0/3] net: more heap allocation and split of rtnl_newlink() Jakub Kicinski
@ 2022-04-29 23:55 ` Jakub Kicinski
2022-04-30 7:32 ` Kalle Valo
2022-04-29 23:55 ` [PATCH net-next 2/3] rtnl: split __rtnl_newlink() into two functions Jakub Kicinski
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2022-04-29 23:55 UTC (permalink / raw)
To: davem; +Cc: netdev, pabeni, edumazet, petrm, Jakub Kicinski, Kalle Valo
Commit a293974590cf ("rtnetlink: avoid frame size warning in rtnl_newlink()")
moved to allocating the largest attribute array of rtnl_newlink()
on the heap. Kalle reports the stack has grown above 1k again:
net/core/rtnetlink.c:3557:1: error: the frame size of 1104 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
Move more attrs to the heap, wrap them in a struct.
Don't bother with linkinfo, it's referenced a lot and we take
its size so it's awkward to move, plus it's small (6 elements).
Reported-by: Kalle Valo <kvalo@kernel.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/core/rtnetlink.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 73f2cbc440c9..33919fd5c202 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3302,17 +3302,23 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
return 0;
}
+struct rtnl_newlink_tbs {
+ struct nlattr *tb[IFLA_MAX + 1];
+ struct nlattr *attr[RTNL_MAX_TYPE + 1];
+ struct nlattr *slave_attr[RTNL_SLAVE_MAX_TYPE + 1];
+};
+
static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
- struct nlattr **attr, struct netlink_ext_ack *extack)
+ struct rtnl_newlink_tbs *tbs,
+ struct netlink_ext_ack *extack)
{
- struct nlattr *slave_attr[RTNL_SLAVE_MAX_TYPE + 1];
unsigned char name_assign_type = NET_NAME_USER;
struct nlattr *linkinfo[IFLA_INFO_MAX + 1];
+ struct nlattr ** const tb = tbs->tb;
const struct rtnl_link_ops *m_ops;
struct net_device *master_dev;
struct net *net = sock_net(skb->sk);
const struct rtnl_link_ops *ops;
- struct nlattr *tb[IFLA_MAX + 1];
struct net *dest_net, *link_net;
struct nlattr **slave_data;
char kind[MODULE_NAME_LEN];
@@ -3382,12 +3388,12 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
return -EINVAL;
if (ops->maxtype && linkinfo[IFLA_INFO_DATA]) {
- err = nla_parse_nested_deprecated(attr, ops->maxtype,
+ err = nla_parse_nested_deprecated(tbs->attr, ops->maxtype,
linkinfo[IFLA_INFO_DATA],
ops->policy, extack);
if (err < 0)
return err;
- data = attr;
+ data = tbs->attr;
}
if (ops->validate) {
err = ops->validate(tb, data, extack);
@@ -3403,14 +3409,14 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
if (m_ops->slave_maxtype &&
linkinfo[IFLA_INFO_SLAVE_DATA]) {
- err = nla_parse_nested_deprecated(slave_attr,
+ err = nla_parse_nested_deprecated(tbs->slave_attr,
m_ops->slave_maxtype,
linkinfo[IFLA_INFO_SLAVE_DATA],
m_ops->slave_policy,
extack);
if (err < 0)
return err;
- slave_data = slave_attr;
+ slave_data = tbs->slave_attr;
}
}
@@ -3559,15 +3565,15 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
struct netlink_ext_ack *extack)
{
- struct nlattr **attr;
+ struct rtnl_newlink_tbs *tbs;
int ret;
- attr = kmalloc_array(RTNL_MAX_TYPE + 1, sizeof(*attr), GFP_KERNEL);
- if (!attr)
+ tbs = kmalloc(sizeof(*tbs), GFP_KERNEL);
+ if (!tbs)
return -ENOMEM;
- ret = __rtnl_newlink(skb, nlh, attr, extack);
- kfree(attr);
+ ret = __rtnl_newlink(skb, nlh, tbs, extack);
+ kfree(tbs);
return ret;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net-next 2/3] rtnl: split __rtnl_newlink() into two functions
2022-04-29 23:55 [PATCH net-next 0/3] net: more heap allocation and split of rtnl_newlink() Jakub Kicinski
2022-04-29 23:55 ` [PATCH net-next 1/3] rtnl: allocate more attr tables on the heap Jakub Kicinski
@ 2022-04-29 23:55 ` Jakub Kicinski
2022-04-29 23:55 ` [PATCH net-next 3/3] rtnl: move rtnl_newlink_create() Jakub Kicinski
2022-05-02 13:30 ` [PATCH net-next 0/3] net: more heap allocation and split of rtnl_newlink() patchwork-bot+netdevbpf
3 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2022-04-29 23:55 UTC (permalink / raw)
To: davem; +Cc: netdev, pabeni, edumazet, petrm, Jakub Kicinski
__rtnl_newlink() is 250LoC, but has a few clear sections.
Move the part which creates a new netdev to a separate
function.
For ease of review code will be moved in the next change.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/core/rtnetlink.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 33919fd5c202..1deef11c6b4d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3302,6 +3302,11 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
return 0;
}
+static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
+ const struct rtnl_link_ops *ops,
+ struct nlattr **tb, struct nlattr **data,
+ struct netlink_ext_ack *extack);
+
struct rtnl_newlink_tbs {
struct nlattr *tb[IFLA_MAX + 1];
struct nlattr *attr[RTNL_MAX_TYPE + 1];
@@ -3312,19 +3317,16 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
struct rtnl_newlink_tbs *tbs,
struct netlink_ext_ack *extack)
{
- unsigned char name_assign_type = NET_NAME_USER;
struct nlattr *linkinfo[IFLA_INFO_MAX + 1];
struct nlattr ** const tb = tbs->tb;
const struct rtnl_link_ops *m_ops;
struct net_device *master_dev;
struct net *net = sock_net(skb->sk);
const struct rtnl_link_ops *ops;
- struct net *dest_net, *link_net;
struct nlattr **slave_data;
char kind[MODULE_NAME_LEN];
struct net_device *dev;
struct ifinfomsg *ifm;
- char ifname[IFNAMSIZ];
struct nlattr **data;
bool link_specified;
int err;
@@ -3484,6 +3486,21 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
return -EOPNOTSUPP;
}
+ return rtnl_newlink_create(skb, ifm, ops, tb, data, extack);
+}
+
+static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
+ const struct rtnl_link_ops *ops,
+ struct nlattr **tb, struct nlattr **data,
+ struct netlink_ext_ack *extack)
+{
+ unsigned char name_assign_type = NET_NAME_USER;
+ struct net *net = sock_net(skb->sk);
+ struct net *dest_net, *link_net;
+ struct net_device *dev;
+ char ifname[IFNAMSIZ];
+ int err;
+
if (!ops->alloc && !ops->setup)
return -EOPNOTSUPP;
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net-next 3/3] rtnl: move rtnl_newlink_create()
2022-04-29 23:55 [PATCH net-next 0/3] net: more heap allocation and split of rtnl_newlink() Jakub Kicinski
2022-04-29 23:55 ` [PATCH net-next 1/3] rtnl: allocate more attr tables on the heap Jakub Kicinski
2022-04-29 23:55 ` [PATCH net-next 2/3] rtnl: split __rtnl_newlink() into two functions Jakub Kicinski
@ 2022-04-29 23:55 ` Jakub Kicinski
2022-05-02 13:30 ` [PATCH net-next 0/3] net: more heap allocation and split of rtnl_newlink() patchwork-bot+netdevbpf
3 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2022-04-29 23:55 UTC (permalink / raw)
To: davem; +Cc: netdev, pabeni, edumazet, petrm, Jakub Kicinski
Pure code move.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/core/rtnetlink.c | 177 +++++++++++++++++++++----------------------
1 file changed, 86 insertions(+), 91 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1deef11c6b4d..eea5ed09e1bb 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3305,7 +3305,92 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
const struct rtnl_link_ops *ops,
struct nlattr **tb, struct nlattr **data,
- struct netlink_ext_ack *extack);
+ struct netlink_ext_ack *extack)
+{
+ unsigned char name_assign_type = NET_NAME_USER;
+ struct net *net = sock_net(skb->sk);
+ struct net *dest_net, *link_net;
+ struct net_device *dev;
+ char ifname[IFNAMSIZ];
+ int err;
+
+ if (!ops->alloc && !ops->setup)
+ return -EOPNOTSUPP;
+
+ if (tb[IFLA_IFNAME]) {
+ nla_strscpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
+ } else {
+ snprintf(ifname, IFNAMSIZ, "%s%%d", ops->kind);
+ name_assign_type = NET_NAME_ENUM;
+ }
+
+ dest_net = rtnl_link_get_net_capable(skb, net, tb, CAP_NET_ADMIN);
+ if (IS_ERR(dest_net))
+ return PTR_ERR(dest_net);
+
+ if (tb[IFLA_LINK_NETNSID]) {
+ int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
+
+ link_net = get_net_ns_by_id(dest_net, id);
+ if (!link_net) {
+ NL_SET_ERR_MSG(extack, "Unknown network namespace id");
+ err = -EINVAL;
+ goto out;
+ }
+ err = -EPERM;
+ if (!netlink_ns_capable(skb, link_net->user_ns, CAP_NET_ADMIN))
+ goto out;
+ } else {
+ link_net = NULL;
+ }
+
+ dev = rtnl_create_link(link_net ? : dest_net, ifname,
+ name_assign_type, ops, tb, extack);
+ if (IS_ERR(dev)) {
+ err = PTR_ERR(dev);
+ goto out;
+ }
+
+ dev->ifindex = ifm->ifi_index;
+
+ if (ops->newlink)
+ err = ops->newlink(link_net ? : net, dev, tb, data, extack);
+ else
+ err = register_netdevice(dev);
+ if (err < 0) {
+ free_netdev(dev);
+ goto out;
+ }
+
+ err = rtnl_configure_link(dev, ifm);
+ if (err < 0)
+ goto out_unregister;
+ if (link_net) {
+ err = dev_change_net_namespace(dev, dest_net, ifname);
+ if (err < 0)
+ goto out_unregister;
+ }
+ if (tb[IFLA_MASTER]) {
+ err = do_set_master(dev, nla_get_u32(tb[IFLA_MASTER]), extack);
+ if (err)
+ goto out_unregister;
+ }
+out:
+ if (link_net)
+ put_net(link_net);
+ put_net(dest_net);
+ return err;
+out_unregister:
+ if (ops->newlink) {
+ LIST_HEAD(list_kill);
+
+ ops->dellink(dev, &list_kill);
+ unregister_netdevice_many(&list_kill);
+ } else {
+ unregister_netdevice(dev);
+ }
+ goto out;
+}
struct rtnl_newlink_tbs {
struct nlattr *tb[IFLA_MAX + 1];
@@ -3489,96 +3574,6 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
return rtnl_newlink_create(skb, ifm, ops, tb, data, extack);
}
-static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
- const struct rtnl_link_ops *ops,
- struct nlattr **tb, struct nlattr **data,
- struct netlink_ext_ack *extack)
-{
- unsigned char name_assign_type = NET_NAME_USER;
- struct net *net = sock_net(skb->sk);
- struct net *dest_net, *link_net;
- struct net_device *dev;
- char ifname[IFNAMSIZ];
- int err;
-
- if (!ops->alloc && !ops->setup)
- return -EOPNOTSUPP;
-
- if (tb[IFLA_IFNAME]) {
- nla_strscpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
- } else {
- snprintf(ifname, IFNAMSIZ, "%s%%d", ops->kind);
- name_assign_type = NET_NAME_ENUM;
- }
-
- dest_net = rtnl_link_get_net_capable(skb, net, tb, CAP_NET_ADMIN);
- if (IS_ERR(dest_net))
- return PTR_ERR(dest_net);
-
- if (tb[IFLA_LINK_NETNSID]) {
- int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
-
- link_net = get_net_ns_by_id(dest_net, id);
- if (!link_net) {
- NL_SET_ERR_MSG(extack, "Unknown network namespace id");
- err = -EINVAL;
- goto out;
- }
- err = -EPERM;
- if (!netlink_ns_capable(skb, link_net->user_ns, CAP_NET_ADMIN))
- goto out;
- } else {
- link_net = NULL;
- }
-
- dev = rtnl_create_link(link_net ? : dest_net, ifname,
- name_assign_type, ops, tb, extack);
- if (IS_ERR(dev)) {
- err = PTR_ERR(dev);
- goto out;
- }
-
- dev->ifindex = ifm->ifi_index;
-
- if (ops->newlink)
- err = ops->newlink(link_net ? : net, dev, tb, data, extack);
- else
- err = register_netdevice(dev);
- if (err < 0) {
- free_netdev(dev);
- goto out;
- }
-
- err = rtnl_configure_link(dev, ifm);
- if (err < 0)
- goto out_unregister;
- if (link_net) {
- err = dev_change_net_namespace(dev, dest_net, ifname);
- if (err < 0)
- goto out_unregister;
- }
- if (tb[IFLA_MASTER]) {
- err = do_set_master(dev, nla_get_u32(tb[IFLA_MASTER]), extack);
- if (err)
- goto out_unregister;
- }
-out:
- if (link_net)
- put_net(link_net);
- put_net(dest_net);
- return err;
-out_unregister:
- if (ops->newlink) {
- LIST_HEAD(list_kill);
-
- ops->dellink(dev, &list_kill);
- unregister_netdevice_many(&list_kill);
- } else {
- unregister_netdevice(dev);
- }
- goto out;
-}
-
static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
struct netlink_ext_ack *extack)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next 0/3] net: more heap allocation and split of rtnl_newlink()
2022-04-29 23:55 [PATCH net-next 0/3] net: more heap allocation and split of rtnl_newlink() Jakub Kicinski
` (2 preceding siblings ...)
2022-04-29 23:55 ` [PATCH net-next 3/3] rtnl: move rtnl_newlink_create() Jakub Kicinski
@ 2022-05-02 13:30 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-05-02 13:30 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev, pabeni, edumazet, petrm
Hello:
This series was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:
On Fri, 29 Apr 2022 16:55:05 -0700 you wrote:
> Small refactoring of rtnl_newlink() to fix a stack usage warning
> and make the function shorter.
>
> Jakub Kicinski (3):
> rtnl: allocate more attr tables on the heap
> rtnl: split __rtnl_newlink() into two functions
> rtnl: move rtnl_newlink_create()
>
> [...]
Here is the summary with links:
- [net-next,1/3] rtnl: allocate more attr tables on the heap
https://git.kernel.org/netdev/net-next/c/c92bf26ccebc
- [net-next,2/3] rtnl: split __rtnl_newlink() into two functions
https://git.kernel.org/netdev/net-next/c/63105e83987a
- [net-next,3/3] rtnl: move rtnl_newlink_create()
https://git.kernel.org/netdev/net-next/c/02839cc8d72b
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread