| Message ID | 20170315031811.22714-1-linus.luessing@c0d3.blue |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
Linus Lüssing <linus.luessing@c0d3.blue> wrote: > When trying to redirect bridged frames to the bridge device itself > via the ebtables nat-prerouting chain and the dnat target then this > currently fails: > > The ethernet destination of the frame is dnat'ed to the MAC address of > the bridge itself just fine and the correctly altered frame can even > be captured via a tcpdump on br0 (with or without promisc mode). > > However, the IP code drops it in the beginning of ip_input.c/ip_rcv() > as the dnat target did not update the skb->pkt_type. Right, thats the reason why ebtables also has ebt_redirect target which does this pkt_type fixup. > - if (dst->is_local) > + if (dst->is_local) { > + /* fix up potential DNAT mess */ > + skb->pkt_type = PACKET_HOST; > + > return br_pass_frame_up(skb); > + } I don't mind this change though (i.e. I don't see how this would bite us later). -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 15, 2017 at 04:18:11AM +0100, Linus Lüssing wrote: > When trying to redirect bridged frames to the bridge device itself > via the ebtables nat-prerouting chain and the dnat target then this > currently fails: > > The ethernet destination of the frame is dnat'ed to the MAC address of > the bridge itself just fine and the correctly altered frame can even > be captured via a tcpdump on br0 (with or without promisc mode). > > However, the IP code drops it in the beginning of ip_input.c/ip_rcv() > as the dnat target did not update the skb->pkt_type. If after > dnat'ing the packet is now destined to us then the skb->pkt_type > needs to be updated from PACKET_OTHERHOST to PACKET_HOST, too. > > Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue> > --- > net/bridge/br_input.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index 013f2290b..ec83175 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -198,8 +198,12 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb > if (dst) { > unsigned long now = jiffies; > > - if (dst->is_local) > + if (dst->is_local) { > + /* fix up potential DNAT mess */ > + skb->pkt_type = PACKET_HOST; I would like to find a way to fix this from ebtables itself, so we don't need to add this code to the bridge core path. AFAICS, from prerouting we don't know the dst yet, so we cannot know if this packet is local from there. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 15, 2017 at 11:26:08AM +0100, Florian Westphal wrote: > Linus Lüssing <linus.luessing@c0d3.blue> wrote: > > When trying to redirect bridged frames to the bridge device itself > > via the ebtables nat-prerouting chain and the dnat target then this > > currently fails: > > > > The ethernet destination of the frame is dnat'ed to the MAC address of > > the bridge itself just fine and the correctly altered frame can even > > be captured via a tcpdump on br0 (with or without promisc mode). > > > > However, the IP code drops it in the beginning of ip_input.c/ip_rcv() > > as the dnat target did not update the skb->pkt_type. > > Right, thats the reason why ebtables also has ebt_redirect target > which does this pkt_type fixup. I'm missing then why redirect is not then just enough for Linus usecase. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 15, 2017 at 11:42:11AM +0100, Pablo Neira Ayuso wrote:
> I'm missing then why redirect is not then just enough for Linus usecase.
For my usecase, the MAC address is configured by the user from a
Web-UI. It may or may not be the one from the bridge device.
Besides, found it counter intuitive that DNAT did not work here
and took me some time to find out why. At least I didn't read about
any such known limitations of the dnat target in the ebtables
manpage.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 15, 2017 at 03:27:20PM +0100, Linus Lüssing wrote: > On Wed, Mar 15, 2017 at 11:42:11AM +0100, Pablo Neira Ayuso wrote: > > I'm missing then why redirect is not then just enough for Linus usecase. > > For my usecase, the MAC address is configured by the user from a > Web-UI. It may or may not be the one from the bridge device. > > Besides, found it counter intuitive that DNAT did not work here > and took me some time to find out why. At least I didn't read about > any such known limitations of the dnat target in the ebtables > manpage. Could you update ebtables dnat to check if the ethernet address matches the one of the input bridge interface, so we mangle the ->pkt_type accordingly from there, instead of doing this from the core? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 15, 2017 at 07:15:39PM +0100, Pablo Neira Ayuso wrote: > Could you update ebtables dnat to check if the ethernet address > matches the one of the input bridge interface, so we mangle the > ->pkt_type accordingly from there, instead of doing this from the > core? Actually, that was the approach I thought about and went for first (and it would probably work for me). Just checking against the bridge device's net_device::dev_addr. I scratched it though, as I was afraid that the issue might still exist for people using some other upper device on top of the bridge device. For instance, macvlan? And iterating over the net_device::dev_addrs list seemed too costly for fast path to me. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 15, 2017 at 10:16:19PM +0100, Linus Lüssing wrote: > On Wed, Mar 15, 2017 at 07:15:39PM +0100, Pablo Neira Ayuso wrote: > > Could you update ebtables dnat to check if the ethernet address > > matches the one of the input bridge interface, so we mangle the > > ->pkt_type accordingly from there, instead of doing this from the > > core? > > Actually, that was the approach I thought about and went for first > (and it would probably work for me). Just checking against the > bridge device's net_device::dev_addr. > > I scratched it though, as I was afraid that the issue might still > exist for people using some other upper device on top of the bridge > device. For instance, macvlan? And iterating over the > net_device::dev_addrs list seemed too costly for fast path to me. I was more thinking of following the simple approach that we follow in ebt_redirect_tg() by taking the input interface. Anyway, I'm ok with this. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 15, 2017 at 11:06:05PM +0100, Pablo Neira Ayuso wrote: > On Wed, Mar 15, 2017 at 10:16:19PM +0100, Linus Lüssing wrote: > > On Wed, Mar 15, 2017 at 07:15:39PM +0100, Pablo Neira Ayuso wrote: > > > Could you update ebtables dnat to check if the ethernet address > > > matches the one of the input bridge interface, so we mangle the > > > ->pkt_type accordingly from there, instead of doing this from the > > > core? > > > > Actually, that was the approach I thought about and went for first > > (and it would probably work for me). Just checking against the > > bridge device's net_device::dev_addr. > > > > I scratched it though, as I was afraid that the issue might still > > exist for people using some other upper device on top of the bridge > > device. For instance, macvlan? And iterating over the > > net_device::dev_addrs list seemed too costly for fast path to me. > > I was more thinking of following the simple approach that we follow in > ebt_redirect_tg() by taking the input interface. > > Anyway, I'm ok with this. Wait. May this break local multicast listener that are bound to the bridge interface? Assuming the bridge interface got an IP address, and that there is local multicast listener. Missing anything here? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 17, 2017 at 02:10:44PM +0100, Pablo Neira Ayuso wrote: > Wait. > > May this break local multicast listener that are bound to the bridge > interface? Assuming the bridge interface got an IP address, and that > there is local multicast listener. > > Missing anything here? Hm, for multicast packets usually the code path a few lines later in br_handle_frame_finish() should be taken instead. But you might be right for IP multicast packets with a unicast MAC destination (due to whatever reason, for instance via DNAT'ing again). Will check that - thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 19, 2017 at 05:55:06PM +0100, Linus Lüssing wrote: > On Fri, Mar 17, 2017 at 02:10:44PM +0100, Pablo Neira Ayuso wrote: > > Wait. > > > > May this break local multicast listener that are bound to the bridge > > interface? Assuming the bridge interface got an IP address, and that > > there is local multicast listener. > > > > Missing anything here? > > Hm, for multicast packets usually the code path a few lines > later in br_handle_frame_finish() should be taken instead. > > But you might be right for IP multicast packets with a unicast MAC > destination (due to whatever reason, for instance via DNAT'ing > again). > > Will check that - thanks! Ok, I tested DNAT'ing an IP multicast packet to the unicast MAC address of the bridge interface. Both ping-ing to an IPv4 and IPv6 multicast listener on br0 worked and was replied to fine, both with or without changing skb->pkt_type from PACKET_MULTICAST to PACKET_HOST. ("$ ping 224.1.0.123" and "$ ping6 ff02::1:ff40:707c%in0" from a network namespace, tied into the bridge via veth) Also, a DNAT'ed PACKET_BROADCAST worked, with or without changing it to PACKET_HOST. I also checked via tcpdump that the destination MAC was changed successfully. So, so far I wasn't able to find any bugs with the current patch. But I think I like the idea of leaving the skb->pkt_type unaltered for PACKET_MULTICAST and PACKET_BROADCAST, seems cleaner. I'd just add an "if (skb->pkt_type == PACKET_OTHERHOST)" check then and resend a PATCH v2. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 21, 2017 at 01:09:47AM +0100, Linus Lüssing wrote: > On Sun, Mar 19, 2017 at 05:55:06PM +0100, Linus Lüssing wrote: > > On Fri, Mar 17, 2017 at 02:10:44PM +0100, Pablo Neira Ayuso wrote: > > > Wait. > > > > > > May this break local multicast listener that are bound to the bridge > > > interface? Assuming the bridge interface got an IP address, and that > > > there is local multicast listener. > > > > > > Missing anything here? > > > > Hm, for multicast packets usually the code path a few lines > > later in br_handle_frame_finish() should be taken instead. > > > > But you might be right for IP multicast packets with a unicast MAC > > destination (due to whatever reason, for instance via DNAT'ing > > again). > > > > Will check that - thanks! > > Ok, I tested DNAT'ing an IP multicast packet to the unicast MAC address > of the bridge interface. > > Both ping-ing to an IPv4 and IPv6 multicast listener on br0 worked > and was replied to fine, both with or without changing skb->pkt_type > from PACKET_MULTICAST to PACKET_HOST. > ("$ ping 224.1.0.123" and "$ ping6 ff02::1:ff40:707c%in0" from a > network namespace, tied into the bridge via veth) > > Also, a DNAT'ed PACKET_BROADCAST worked, with or without changing > it to PACKET_HOST. > > I also checked via tcpdump that the destination MAC was changed > successfully. Thanks for looking into this more in depth. > So, so far I wasn't able to find any bugs with the current > patch. But I think I like the idea of leaving the skb->pkt_type > unaltered for PACKET_MULTICAST and PACKET_BROADCAST, seems cleaner. Yes, and matching on skb->pkt_type would not break from netfilter. > I'd just add an "if (skb->pkt_type == PACKET_OTHERHOST)" check > then and resend a PATCH v2. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index 013f2290b..ec83175 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -198,8 +198,12 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb if (dst) { unsigned long now = jiffies; - if (dst->is_local) + if (dst->is_local) { + /* fix up potential DNAT mess */ + skb->pkt_type = PACKET_HOST; + return br_pass_frame_up(skb); + } if (now != dst->used) dst->used = now;
When trying to redirect bridged frames to the bridge device itself via the ebtables nat-prerouting chain and the dnat target then this currently fails: The ethernet destination of the frame is dnat'ed to the MAC address of the bridge itself just fine and the correctly altered frame can even be captured via a tcpdump on br0 (with or without promisc mode). However, the IP code drops it in the beginning of ip_input.c/ip_rcv() as the dnat target did not update the skb->pkt_type. If after dnat'ing the packet is now destined to us then the skb->pkt_type needs to be updated from PACKET_OTHERHOST to PACKET_HOST, too. Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue> --- net/bridge/br_input.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)