| Message ID | 20240110110451.5473-2-ptikhomirov@virtuozzo.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netlink: bridge: fix nf_bridge->physindev use after free | expand |
Pavel Tikhomirov <ptikhomirov@virtuozzo.com> wrote: > We don't use physindev in __build_packet_message except for getting > physinif from it. So let's switch to nf_bridge_get_physinif to get what > we want directly. > > Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com> > --- > net/netfilter/nfnetlink_log.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c > index f03f4d4d7d889..134e05d31061e 100644 > --- a/net/netfilter/nfnetlink_log.c > +++ b/net/netfilter/nfnetlink_log.c > @@ -508,7 +508,7 @@ __build_packet_message(struct nfnl_log_net *log, > htonl(br_port_get_rcu(indev)->br->dev->ifindex))) > goto nla_put_failure; > } else { > - struct net_device *physindev; > + int physinif; > > /* Case 2: indev is bridge group, we need to look for > * physical device (when called from ipv4) */ > @@ -516,10 +516,10 @@ __build_packet_message(struct nfnl_log_net *log, > htonl(indev->ifindex))) > goto nla_put_failure; > > - physindev = nf_bridge_get_physindev(skb); > - if (physindev && > + physinif = nf_bridge_get_physinif(skb); > + if (physinif && > nla_put_be32(inst->skb, NFULA_IFINDEX_PHYSINDEV, > - htonl(physindev->ifindex))) > + htonl(physinif))) I think you can drop this patch and make the last patch pass nf_bridge_info->physinif directly.
On 10/01/2024 21:33, Florian Westphal wrote: > Pavel Tikhomirov <ptikhomirov@virtuozzo.com> wrote: >> We don't use physindev in __build_packet_message except for getting >> physinif from it. So let's switch to nf_bridge_get_physinif to get what >> we want directly. >> >> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com> >> --- >> net/netfilter/nfnetlink_log.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c >> index f03f4d4d7d889..134e05d31061e 100644 >> --- a/net/netfilter/nfnetlink_log.c >> +++ b/net/netfilter/nfnetlink_log.c >> @@ -508,7 +508,7 @@ __build_packet_message(struct nfnl_log_net *log, >> htonl(br_port_get_rcu(indev)->br->dev->ifindex))) >> goto nla_put_failure; >> } else { >> - struct net_device *physindev; >> + int physinif; >> >> /* Case 2: indev is bridge group, we need to look for >> * physical device (when called from ipv4) */ >> @@ -516,10 +516,10 @@ __build_packet_message(struct nfnl_log_net *log, >> htonl(indev->ifindex))) >> goto nla_put_failure; >> >> - physindev = nf_bridge_get_physindev(skb); >> - if (physindev && >> + physinif = nf_bridge_get_physinif(skb); >> + if (physinif && >> nla_put_be32(inst->skb, NFULA_IFINDEX_PHYSINDEV, >> - htonl(physindev->ifindex))) >> + htonl(physinif))) > > I think you can drop this patch and make the last patch pass > nf_bridge_info->physinif directly. The whole Idea of this patch was to replace nf_bridge_get_physindev with nf_bridge_get_physinif before the patch which propagates net, so that we don't need to propagate net first and then in later patch remove it when replacing with nf_bridge_get_physinif. But I spoiled it by forgetting to remove net propagation to __build_packet_message... Is it ok if I leave this patch as is, but instead remove: diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c index 134e05d31061e..ad93dd77e6071 100644 --- a/net/netfilter/nfnetlink_log.c +++ b/net/netfilter/nfnetlink_log.c @@ -463,7 +463,8 @@ __build_packet_message(struct nfnl_log_net *log, const struct net_device *outdev, const char *prefix, unsigned int plen, const struct nfnl_ct_hook *nfnl_ct, - struct nf_conn *ct, enum ip_conntrack_info ctinfo) + struct nf_conn *ct, enum ip_conntrack_info ctinfo, + struct net *net) { struct nfulnl_msg_packet_hdr pmsg; struct nlmsghdr *nlh; @@ -804,7 +805,7 @@ nfulnl_log_packet(struct net *net, __build_packet_message(log, inst, skb, data_len, pf, hooknum, in, out, prefix, plen, - nfnl_ct, ct, ctinfo); + nfnl_ct, ct, ctinfo, net); if (inst->qlen >= qthreshold) __nfulnl_flush(inst); from third patch?
Pavel Tikhomirov <ptikhomirov@virtuozzo.com> wrote: > On 10/01/2024 21:33, Florian Westphal wrote: > > Pavel Tikhomirov <ptikhomirov@virtuozzo.com> wrote: > > I think you can drop this patch and make the last patch pass > > nf_bridge_info->physinif directly. > > The whole Idea of this patch was to replace nf_bridge_get_physindev with > nf_bridge_get_physinif before the patch which propagates net, so that we > don't need to propagate net first and then in later patch remove it when > replacing with nf_bridge_get_physinif. > > But I spoiled it by forgetting to remove net propagation to > __build_packet_message... > > Is it ok if I leave this patch as is, but instead remove: Yes, thats fine.
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c index f03f4d4d7d889..134e05d31061e 100644 --- a/net/netfilter/nfnetlink_log.c +++ b/net/netfilter/nfnetlink_log.c @@ -508,7 +508,7 @@ __build_packet_message(struct nfnl_log_net *log, htonl(br_port_get_rcu(indev)->br->dev->ifindex))) goto nla_put_failure; } else { - struct net_device *physindev; + int physinif; /* Case 2: indev is bridge group, we need to look for * physical device (when called from ipv4) */ @@ -516,10 +516,10 @@ __build_packet_message(struct nfnl_log_net *log, htonl(indev->ifindex))) goto nla_put_failure; - physindev = nf_bridge_get_physindev(skb); - if (physindev && + physinif = nf_bridge_get_physinif(skb); + if (physinif && nla_put_be32(inst->skb, NFULA_IFINDEX_PHYSINDEV, - htonl(physindev->ifindex))) + htonl(physinif))) goto nla_put_failure; } #endif
We don't use physindev in __build_packet_message except for getting physinif from it. So let's switch to nf_bridge_get_physinif to get what we want directly. Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com> --- net/netfilter/nfnetlink_log.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)