Rafael Zalamena <[email protected]> writes:
> On Wed, Dec 07, 2016 at 09:36:24PM +0100, Jeremie Courreges-Anglas wrote:
>> Rafael Zalamena <[email protected]> writes:
>>
>> > I'm implementing some features for dhcrelay and to make them fit I need
>> > some clean ups in the dhcrelay(8) first. This diff changes most of the
>> > input/output functions prototypes to take one parameter with all addresses
>> > instead of passing multiple parameters.
>> >
>> > Basically this will make input functions gather more information (source/
>> > destination MACs, source/destination IPs, source/destination ports) and
>> > use it in the output instead of trying to figure out this information along
>> > the way.
>> >
>> > With this we will be able to add IPv6 support and layer 2 relaying.
>>
>> Nice. :)
>>
>> [...]
>>
>> > ok?
>>
>> This conflicts with a diff that has been committed by patrick@, you'll
>> need to refresh it.
>
> I updated the diff with the latest commits from patrick@. Basically instead
> of cleaning hto to trigger the memset(, 0xff,) on destination mac we just
> update the pc dmac field (see assemble_hw_header()).
Thanks!
>>
>> I didn't review it entirely, but please address the point below.
>
> I changed the struct fields sss and dss to src and dst respectively.
>
> ok?
I think you've summoned Cthulhu with this diff. :)
I started noticing a wrong behavior with when testing a multi-relay
setup, as prompted by Patrick's diff. See
marc.info/?l=openbsd-tech&m=148096893918967&w=2
Such as setup didn't work before, and is still half-broken in -current.
For the record:
--8<--
Addresses start with 192.168...
[server] <-> [relay1] <-> [relay2] <-> [client]
1.1 1.2 2.1 2.2 3.1
Here dhcrelay on relay2 ignores replies sent by 192.168.1.1 to
192.168.3.1, because its UDP socket is bound to 192.168.3.1 but
connected to 192.168.2.2.
-->8--
A second problem is that, if [relay1] is on the path between [server] and
[relay2], the BPF socket will catch [server]'s reply, think it has to
handle it (I don't think it should), and send it to [client]. The code
in dhcrelay(8) doesn't make enough checks to cope with such a situation.
This is not a big problem in practice, but now your diff comes in.
In got_response(), you store client_port, 68, in
ss2sin(&pc->pc_dst)->sin_port. But in got_one() -> receive_packet() ->
decode_udp_ip_header(), you store the UDP port of the incoming packet.
In case of a [server]->[relay2] messages, the port is 67. [relay1] thus
sends a packet to the client with 67 as the destination port. This
packet matches the BPF filter of [relay1], which then goes in an
infinite loop.
IIUC, an obvious improvement would be to ignore BOOTREPLY packets with
a giaddr that doesn't match our primary address. relay() could use such
a check:
/* If it's a bootreply, forward it to the client. */
if (packet->op == BOOTREPLY) {
/* Is this packet actually for us? */
if (packet->giaddr.s_addr != interfaces->primary_address.s_addr)
return;
[...]
}
Maybe there is a drawback that I can't see, but blindly relaying like we
do now can't be a good thing.
The reason why port 67 ends up on the wire instead of port 68 is
probably this:
bzero(&to, sizeof(to));
if (!(packet->flags & htons(BOOTP_BROADCAST))) {
to.sin_addr = packet->yiaddr;
to.sin_port = client_port;
} else {
to.sin_addr.s_addr = htonl(INADDR_BROADCAST);
to.sin_port = client_port;
}
to.sin_family = AF_INET;
to.sin_len = sizeof to;
memcpy(&ss2sin(&pc->pc_dst)->sin_addr, &to.sin_addr,
sizeof(ss2sin(&pc->pc_dst)->sin_addr));
The memcpy copies only the IP address, not the port. This should
be something like
*ss2sin(&pc->pc_dst) = to;
Another problem: the relay->server code uses send(2) on a connected
socket and thus has no destination IP issue. But the relay->client path
now uses the source address from the server->relay packet. I think
we should keep using interfaces->primary_address here...
No idea if the proposals above would fix all the problems, but I have to
finish this mail. :)
Aside from this:
- please drop the extra netinet/if_ether.h includes
- not a problem per se, but such a simple function like ss2sin could
probably be defined static inline in a header
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
