* [PATCH net 0/3] rtnetlink: a couple of fixes in linkmsg validation
@ 2023-05-25 21:49 Xin Long
2023-05-25 21:49 ` [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link Xin Long
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Xin Long @ 2023-05-25 21:49 UTC (permalink / raw)
To: network dev
Cc: davem, kuba, Eric Dumazet, Paolo Abeni, Thomas Graf, Alexander Duyck
validate_linkmsg() was introduced to do linkmsg validation for existing
links. However, the new created links also need this linkmsg validation.
Move it to the correct place in Patch 1, and add more tb checks in
Patch 2 and 3.
Xin Long (3):
rtnetlink: move validate_linkmsg into rtnl_create_link
rtnetlink: move IFLA_GSO_ tb check to validate_linkmsg
rtnetlink: add the missing IFLA_GRO_ tb check in validate_linkmsg
net/core/rtnetlink.c | 70 +++++++++++++++++++++++++++-----------------
1 file changed, 43 insertions(+), 27 deletions(-)
--
2.39.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link
2023-05-25 21:49 [PATCH net 0/3] rtnetlink: a couple of fixes in linkmsg validation Xin Long
@ 2023-05-25 21:49 ` Xin Long
2023-05-26 10:40 ` Simon Horman
2023-05-27 3:49 ` Jakub Kicinski
2023-05-25 21:49 ` [PATCH net 2/3] rtnetlink: move IFLA_GSO_ tb check to validate_linkmsg Xin Long
2023-05-25 21:49 ` [PATCH net 3/3] rtnetlink: add the missing IFLA_GRO_ tb check in validate_linkmsg Xin Long
2 siblings, 2 replies; 10+ messages in thread
From: Xin Long @ 2023-05-25 21:49 UTC (permalink / raw)
To: network dev
Cc: davem, kuba, Eric Dumazet, Paolo Abeni, Thomas Graf, Alexander Duyck
In commit 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()"),
it moved validate_linkmsg() from rtnl_setlink() to do_setlink(). However,
as validate_linkmsg() is also called in __rtnl_newlink(), it caused
validate_linkmsg() being called twice when running 'ip link set'.
The validate_linkmsg() was introduced by commit 1840bb13c22f5b ("[RTNL]:
Validate hardware and broadcast address attribute for RTM_NEWLINK") for
existing links. After adding it in do_setlink(), there's no need to call
it in __rtnl_newlink().
Instead of deleting it from __rtnl_newlink(), this patch moves it to
rtnl_create_link() to fix the missing validation for the new created
links.
Fixes: 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/core/rtnetlink.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 653901a1bf75..d1f88ba7e391 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2377,15 +2377,13 @@ static int rtnl_set_vf_rate(struct net_device *dev, int vf, int min_tx_rate,
static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[],
struct netlink_ext_ack *extack)
{
- if (dev) {
- if (tb[IFLA_ADDRESS] &&
- nla_len(tb[IFLA_ADDRESS]) < dev->addr_len)
- return -EINVAL;
+ if (tb[IFLA_ADDRESS] &&
+ nla_len(tb[IFLA_ADDRESS]) < dev->addr_len)
+ return -EINVAL;
- if (tb[IFLA_BROADCAST] &&
- nla_len(tb[IFLA_BROADCAST]) < dev->addr_len)
- return -EINVAL;
- }
+ if (tb[IFLA_BROADCAST] &&
+ nla_len(tb[IFLA_BROADCAST]) < dev->addr_len)
+ return -EINVAL;
if (tb[IFLA_AF_SPEC]) {
struct nlattr *af;
@@ -3285,6 +3283,7 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
struct net_device *dev;
unsigned int num_tx_queues = 1;
unsigned int num_rx_queues = 1;
+ int err;
if (tb[IFLA_NUM_TX_QUEUES])
num_tx_queues = nla_get_u32(tb[IFLA_NUM_TX_QUEUES]);
@@ -3320,13 +3319,18 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
if (!dev)
return ERR_PTR(-ENOMEM);
+ err = validate_linkmsg(dev, tb, extack);
+ if (err < 0) {
+ free_netdev(dev);
+ return ERR_PTR(err);
+ }
+
dev_net_set(dev, net);
dev->rtnl_link_ops = ops;
dev->rtnl_link_state = RTNL_LINK_INITIALIZING;
if (tb[IFLA_MTU]) {
u32 mtu = nla_get_u32(tb[IFLA_MTU]);
- int err;
err = dev_validate_mtu(dev, mtu, extack);
if (err) {
@@ -3534,10 +3538,6 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
m_ops = master_dev->rtnl_link_ops;
}
- err = validate_linkmsg(dev, tb, extack);
- if (err < 0)
- return err;
-
if (tb[IFLA_LINKINFO]) {
err = nla_parse_nested_deprecated(linkinfo, IFLA_INFO_MAX,
tb[IFLA_LINKINFO],
--
2.39.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net 2/3] rtnetlink: move IFLA_GSO_ tb check to validate_linkmsg
2023-05-25 21:49 [PATCH net 0/3] rtnetlink: a couple of fixes in linkmsg validation Xin Long
2023-05-25 21:49 ` [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link Xin Long
@ 2023-05-25 21:49 ` Xin Long
2023-05-26 10:40 ` Simon Horman
2023-05-25 21:49 ` [PATCH net 3/3] rtnetlink: add the missing IFLA_GRO_ tb check in validate_linkmsg Xin Long
2 siblings, 1 reply; 10+ messages in thread
From: Xin Long @ 2023-05-25 21:49 UTC (permalink / raw)
To: network dev
Cc: davem, kuba, Eric Dumazet, Paolo Abeni, Thomas Graf, Alexander Duyck
These IFLA_GSO_* tb check should also be done for the new created link,
otherwise, they can be set to a huge value when creating links:
# ip link add dummy1 gso_max_size 4294967295 type dummy
# ip -d link show dummy1
dummy addrgenmode eui64 ... gso_max_size 4294967295
Fixes: 46e6b992c250 ("rtnetlink: allow GSO maximums to be set on device creation")
Fixes: 9eefedd58ae1 ("net: add gso_ipv4_max_size and gro_ipv4_max_size per device")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/core/rtnetlink.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d1f88ba7e391..68a58d0a7b79 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2385,6 +2385,25 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[],
nla_len(tb[IFLA_BROADCAST]) < dev->addr_len)
return -EINVAL;
+ if (tb[IFLA_GSO_MAX_SIZE] &&
+ nla_get_u32(tb[IFLA_GSO_MAX_SIZE]) > dev->tso_max_size) {
+ NL_SET_ERR_MSG(extack, "too big gso_max_size");
+ return -EINVAL;
+ }
+
+ if (tb[IFLA_GSO_MAX_SEGS] &&
+ (nla_get_u32(tb[IFLA_GSO_MAX_SEGS]) > GSO_MAX_SEGS ||
+ nla_get_u32(tb[IFLA_GSO_MAX_SEGS]) > dev->tso_max_segs)) {
+ NL_SET_ERR_MSG(extack, "too big gso_max_segs");
+ return -EINVAL;
+ }
+
+ if (tb[IFLA_GSO_IPV4_MAX_SIZE] &&
+ nla_get_u32(tb[IFLA_GSO_IPV4_MAX_SIZE]) > dev->tso_max_size) {
+ NL_SET_ERR_MSG(extack, "too big gso_ipv4_max_size");
+ return -EINVAL;
+ }
+
if (tb[IFLA_AF_SPEC]) {
struct nlattr *af;
int rem, err;
@@ -2856,11 +2875,6 @@ static int do_setlink(const struct sk_buff *skb,
if (tb[IFLA_GSO_MAX_SIZE]) {
u32 max_size = nla_get_u32(tb[IFLA_GSO_MAX_SIZE]);
- if (max_size > dev->tso_max_size) {
- err = -EINVAL;
- goto errout;
- }
-
if (dev->gso_max_size ^ max_size) {
netif_set_gso_max_size(dev, max_size);
status |= DO_SETLINK_MODIFIED;
@@ -2870,11 +2884,6 @@ static int do_setlink(const struct sk_buff *skb,
if (tb[IFLA_GSO_MAX_SEGS]) {
u32 max_segs = nla_get_u32(tb[IFLA_GSO_MAX_SEGS]);
- if (max_segs > GSO_MAX_SEGS || max_segs > dev->tso_max_segs) {
- err = -EINVAL;
- goto errout;
- }
-
if (dev->gso_max_segs ^ max_segs) {
netif_set_gso_max_segs(dev, max_segs);
status |= DO_SETLINK_MODIFIED;
@@ -2893,11 +2902,6 @@ static int do_setlink(const struct sk_buff *skb,
if (tb[IFLA_GSO_IPV4_MAX_SIZE]) {
u32 max_size = nla_get_u32(tb[IFLA_GSO_IPV4_MAX_SIZE]);
- if (max_size > dev->tso_max_size) {
- err = -EINVAL;
- goto errout;
- }
-
if (dev->gso_ipv4_max_size ^ max_size) {
netif_set_gso_ipv4_max_size(dev, max_size);
status |= DO_SETLINK_MODIFIED;
--
2.39.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net 3/3] rtnetlink: add the missing IFLA_GRO_ tb check in validate_linkmsg
2023-05-25 21:49 [PATCH net 0/3] rtnetlink: a couple of fixes in linkmsg validation Xin Long
2023-05-25 21:49 ` [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link Xin Long
2023-05-25 21:49 ` [PATCH net 2/3] rtnetlink: move IFLA_GSO_ tb check to validate_linkmsg Xin Long
@ 2023-05-25 21:49 ` Xin Long
2023-05-26 10:41 ` Simon Horman
2 siblings, 1 reply; 10+ messages in thread
From: Xin Long @ 2023-05-25 21:49 UTC (permalink / raw)
To: network dev
Cc: davem, kuba, Eric Dumazet, Paolo Abeni, Thomas Graf, Alexander Duyck
This fixes the issue that dev gro_max_size and gso_ipv4_max_size
can be set to a huge value:
# ip link add dummy1 type dummy
# ip link set dummy1 gro_max_size 4294967295
# ip -d link show dummy1 |grep gro_max_size
dummy addrgenmode eui64 ... gro_max_size 4294967295
Fixes: 0fe79f28bfaf ("net: allow gro_max_size to exceed 65536")
Fixes: 9eefedd58ae1 ("net: add gso_ipv4_max_size and gro_ipv4_max_size per device")
Reported-by: Xiumei Mu <xmu@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/core/rtnetlink.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 68a58d0a7b79..4c7d84072bd4 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2398,12 +2398,24 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[],
return -EINVAL;
}
+ if (tb[IFLA_GRO_MAX_SIZE] &&
+ nla_get_u32(tb[IFLA_GRO_MAX_SIZE]) > GRO_MAX_SIZE) {
+ NL_SET_ERR_MSG(extack, "too big gro_max_size");
+ return -EINVAL;
+ }
+
if (tb[IFLA_GSO_IPV4_MAX_SIZE] &&
nla_get_u32(tb[IFLA_GSO_IPV4_MAX_SIZE]) > dev->tso_max_size) {
NL_SET_ERR_MSG(extack, "too big gso_ipv4_max_size");
return -EINVAL;
}
+ if (tb[IFLA_GRO_IPV4_MAX_SIZE] &&
+ nla_get_u32(tb[IFLA_GRO_IPV4_MAX_SIZE]) > GRO_MAX_SIZE) {
+ NL_SET_ERR_MSG(extack, "too big gro_ipv4_max_size");
+ return -EINVAL;
+ }
+
if (tb[IFLA_AF_SPEC]) {
struct nlattr *af;
int rem, err;
--
2.39.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link
2023-05-25 21:49 ` [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link Xin Long
@ 2023-05-26 10:40 ` Simon Horman
2023-05-27 3:49 ` Jakub Kicinski
1 sibling, 0 replies; 10+ messages in thread
From: Simon Horman @ 2023-05-26 10:40 UTC (permalink / raw)
To: Xin Long
Cc: network dev, davem, kuba, Eric Dumazet, Paolo Abeni, Thomas Graf,
Alexander Duyck
On Thu, May 25, 2023 at 05:49:15PM -0400, Xin Long wrote:
> In commit 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()"),
> it moved validate_linkmsg() from rtnl_setlink() to do_setlink(). However,
> as validate_linkmsg() is also called in __rtnl_newlink(), it caused
> validate_linkmsg() being called twice when running 'ip link set'.
>
> The validate_linkmsg() was introduced by commit 1840bb13c22f5b ("[RTNL]:
> Validate hardware and broadcast address attribute for RTM_NEWLINK") for
> existing links. After adding it in do_setlink(), there's no need to call
> it in __rtnl_newlink().
>
> Instead of deleting it from __rtnl_newlink(), this patch moves it to
> rtnl_create_link() to fix the missing validation for the new created
> links.
>
> Fixes: 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 2/3] rtnetlink: move IFLA_GSO_ tb check to validate_linkmsg
2023-05-25 21:49 ` [PATCH net 2/3] rtnetlink: move IFLA_GSO_ tb check to validate_linkmsg Xin Long
@ 2023-05-26 10:40 ` Simon Horman
0 siblings, 0 replies; 10+ messages in thread
From: Simon Horman @ 2023-05-26 10:40 UTC (permalink / raw)
To: Xin Long
Cc: network dev, davem, kuba, Eric Dumazet, Paolo Abeni, Thomas Graf,
Alexander Duyck
On Thu, May 25, 2023 at 05:49:16PM -0400, Xin Long wrote:
> These IFLA_GSO_* tb check should also be done for the new created link,
> otherwise, they can be set to a huge value when creating links:
>
> # ip link add dummy1 gso_max_size 4294967295 type dummy
> # ip -d link show dummy1
> dummy addrgenmode eui64 ... gso_max_size 4294967295
>
> Fixes: 46e6b992c250 ("rtnetlink: allow GSO maximums to be set on device creation")
> Fixes: 9eefedd58ae1 ("net: add gso_ipv4_max_size and gro_ipv4_max_size per device")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 3/3] rtnetlink: add the missing IFLA_GRO_ tb check in validate_linkmsg
2023-05-25 21:49 ` [PATCH net 3/3] rtnetlink: add the missing IFLA_GRO_ tb check in validate_linkmsg Xin Long
@ 2023-05-26 10:41 ` Simon Horman
0 siblings, 0 replies; 10+ messages in thread
From: Simon Horman @ 2023-05-26 10:41 UTC (permalink / raw)
To: Xin Long
Cc: network dev, davem, kuba, Eric Dumazet, Paolo Abeni, Thomas Graf,
Alexander Duyck
On Thu, May 25, 2023 at 05:49:17PM -0400, Xin Long wrote:
> This fixes the issue that dev gro_max_size and gso_ipv4_max_size
> can be set to a huge value:
>
> # ip link add dummy1 type dummy
> # ip link set dummy1 gro_max_size 4294967295
> # ip -d link show dummy1 |grep gro_max_size
> dummy addrgenmode eui64 ... gro_max_size 4294967295
>
> Fixes: 0fe79f28bfaf ("net: allow gro_max_size to exceed 65536")
> Fixes: 9eefedd58ae1 ("net: add gso_ipv4_max_size and gro_ipv4_max_size per device")
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link
2023-05-25 21:49 ` [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link Xin Long
2023-05-26 10:40 ` Simon Horman
@ 2023-05-27 3:49 ` Jakub Kicinski
2023-05-27 20:36 ` Xin Long
1 sibling, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2023-05-27 3:49 UTC (permalink / raw)
To: Xin Long
Cc: network dev, davem, Eric Dumazet, Paolo Abeni, Thomas Graf,
Alexander Duyck
On Thu, 25 May 2023 17:49:15 -0400 Xin Long wrote:
> In commit 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()"),
> it moved validate_linkmsg() from rtnl_setlink() to do_setlink(). However,
> as validate_linkmsg() is also called in __rtnl_newlink(), it caused
> validate_linkmsg() being called twice when running 'ip link set'.
>
> The validate_linkmsg() was introduced by commit 1840bb13c22f5b ("[RTNL]:
> Validate hardware and broadcast address attribute for RTM_NEWLINK") for
> existing links. After adding it in do_setlink(), there's no need to call
> it in __rtnl_newlink().
>
> Instead of deleting it from __rtnl_newlink(), this patch moves it to
> rtnl_create_link() to fix the missing validation for the new created
> links.
>
> Fixes: 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()")
I don't see any bug in here, is there one? Or you're just trying
to avoid calling validation twice? I think it's better to validate
twice than validate after some changes have already been applied
by __rtnl_newlink()... If we really care about the double validation
we should pull the validation out of do_setlink(), IMHO.
--
pw-bot: cr
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link
2023-05-27 3:49 ` Jakub Kicinski
@ 2023-05-27 20:36 ` Xin Long
2023-05-30 1:25 ` Jakub Kicinski
0 siblings, 1 reply; 10+ messages in thread
From: Xin Long @ 2023-05-27 20:36 UTC (permalink / raw)
To: Jakub Kicinski
Cc: network dev, davem, Eric Dumazet, Paolo Abeni, Thomas Graf,
Alexander Duyck
On Fri, May 26, 2023 at 11:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 25 May 2023 17:49:15 -0400 Xin Long wrote:
> > In commit 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()"),
> > it moved validate_linkmsg() from rtnl_setlink() to do_setlink(). However,
> > as validate_linkmsg() is also called in __rtnl_newlink(), it caused
> > validate_linkmsg() being called twice when running 'ip link set'.
> >
> > The validate_linkmsg() was introduced by commit 1840bb13c22f5b ("[RTNL]:
> > Validate hardware and broadcast address attribute for RTM_NEWLINK") for
> > existing links. After adding it in do_setlink(), there's no need to call
> > it in __rtnl_newlink().
> >
> > Instead of deleting it from __rtnl_newlink(), this patch moves it to
> > rtnl_create_link() to fix the missing validation for the new created
> > links.
> >
> > Fixes: 644c7eebbfd5 ("rtnetlink: validate attributes in do_setlink()")
>
> I don't see any bug in here, is there one? Or you're just trying
> to avoid calling validation twice? I think it's better to validate
> twice than validate after some changes have already been applied
> by __rtnl_newlink()... If we really care about the double validation
> we should pull the validation out of do_setlink(), IMHO.
Other than avoiding calling validation twice, adding validate_linkmsg() in
rtnl_create_link() also fixes the missing validation for newly created links,
it includes tb[IFLA_ADDRESS/IFLA_BROADCAST] checks in validate_linkmsg():
For example, because of validate_linkmsg(), these command will fail
# ip link add dummy0 type dummy
# ip link add link dummy0 name mac0 type macsec
# ip link set mac0 address 01
RTNETLINK answers: Invalid argument
(I removed the IFLA_ADDRESS validation in iproute2 itself)
However, the below will work:
# ip link add dummy0 type dummy
# ip link add link dummy0 name mac0 address 01 type macsec
As for the calling twice thing, validating before any changes happen
makes sense.
Based on the change in this patch, I will pull the validation out of
do_setlink()
to these 3 places:
@@ -3600,7 +3605,9 @@ static int __rtnl_newlink(struct sk_buff *skb,
struct nlmsghdr *nlh,
return -EEXIST;
if (nlh->nlmsg_flags & NLM_F_REPLACE)
return -EOPNOTSUPP;
-
+ err = validate_linkmsg(dev, tb, extack);
+ if (err < 0)
+ return err;
@@ -3377,6 +3383,9 @@ static int rtnl_group_changelink(const struct
sk_buff *skb,
for_each_netdev_safe(net, dev, aux) {
if (dev->group == group) {
+ err = validate_linkmsg(dev, tb, extack);
+ if (err < 0)
+ return err;
err = do_setlink(skb, dev, ifm, extack, tb, 0);
@@ -3140,6 +3136,10 @@ static int rtnl_setlink(struct sk_buff *skb,
struct nlmsghdr *nlh,
goto errout;
}
+ err = validate_linkmsg(dev, tb, extack);
+ if (err < 0)
+ goto errout;
+
err = do_setlink(skb, dev, ifm, extack, tb, 0);
and yes, one more place calls validate_linkmsg (comparing to the old code
with the one in rtnl_create_link), unless someone thinks it's not worth it.
Thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link
2023-05-27 20:36 ` Xin Long
@ 2023-05-30 1:25 ` Jakub Kicinski
0 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2023-05-30 1:25 UTC (permalink / raw)
To: Xin Long
Cc: network dev, davem, Eric Dumazet, Paolo Abeni, Thomas Graf,
Alexander Duyck
On Sat, 27 May 2023 16:36:15 -0400 Xin Long wrote:
> Other than avoiding calling validation twice, adding validate_linkmsg() in
> rtnl_create_link() also fixes the missing validation for newly created links,
> it includes tb[IFLA_ADDRESS/IFLA_BROADCAST] checks in validate_linkmsg():
Ah, I see. Since this is a fix I'd err on the side of keeping the
change small and obvious and limit it to adding the call in
validate_linkmsg() without worrying about validating multiple times.
Then follow up with the refactoring to net-next.
I guess it could be acceptable to take the whole thing into net, if
you prefer but at least the commit message would need clarification.
> As for the calling twice thing, validating before any changes happen
> makes sense.
> Based on the change in this patch, I will pull the validation out of
> do_setlink()
> to these 3 places:
>
> @@ -3600,7 +3605,9 @@ static int __rtnl_newlink(struct sk_buff *skb,
> struct nlmsghdr *nlh,
> return -EEXIST;
> if (nlh->nlmsg_flags & NLM_F_REPLACE)
> return -EOPNOTSUPP;
> -
> + err = validate_linkmsg(dev, tb, extack);
> + if (err < 0)
> + return err;
>
> @@ -3377,6 +3383,9 @@ static int rtnl_group_changelink(const struct
> sk_buff *skb,
>
> for_each_netdev_safe(net, dev, aux) {
> if (dev->group == group) {
> + err = validate_linkmsg(dev, tb, extack);
> + if (err < 0)
> + return err;
> err = do_setlink(skb, dev, ifm, extack, tb, 0);
>
> @@ -3140,6 +3136,10 @@ static int rtnl_setlink(struct sk_buff *skb,
> struct nlmsghdr *nlh,
> goto errout;
> }
>
> + err = validate_linkmsg(dev, tb, extack);
> + if (err < 0)
> + goto errout;
> +
> err = do_setlink(skb, dev, ifm, extack, tb, 0);
>
>
> and yes, one more place calls validate_linkmsg (comparing to the old code
> with the one in rtnl_create_link), unless someone thinks it's not worth it.
Yup, LGTM. Renaming do_setlink() to __do_setlink() and adding a wrapper
called do_setlink() which does the validation and calls __do_setlink() -
seems like another option to explore. Whatever you reckon ends up
looking neatest. As long as the validation ends up moving towards
the entry point not deeper in - any approach is fine.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-05-30 1:25 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-25 21:49 [PATCH net 0/3] rtnetlink: a couple of fixes in linkmsg validation Xin Long
2023-05-25 21:49 ` [PATCH net 1/3] rtnetlink: move validate_linkmsg into rtnl_create_link Xin Long
2023-05-26 10:40 ` Simon Horman
2023-05-27 3:49 ` Jakub Kicinski
2023-05-27 20:36 ` Xin Long
2023-05-30 1:25 ` Jakub Kicinski
2023-05-25 21:49 ` [PATCH net 2/3] rtnetlink: move IFLA_GSO_ tb check to validate_linkmsg Xin Long
2023-05-26 10:40 ` Simon Horman
2023-05-25 21:49 ` [PATCH net 3/3] rtnetlink: add the missing IFLA_GRO_ tb check in validate_linkmsg Xin Long
2023-05-26 10:41 ` Simon Horman
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.