Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On Mon, Jun 2, 2014 at 2:09 PM, Patrik Flykt patrik.fl...@linux.intel.com wrote: On Fri, 2014-05-30 at 17:21 +0100, Tom Gundersen wrote: I'm wondering if the criterion should be to request broadcast if and only if we have not configured an IP address (I.e. only in discovering, requesting and init-reboot), as that seems to be the problem, or did I get that wrong? Yes, I was aiming at requesting broadcast delivery from the server only for broadcast packets sent by the client. That can be an overly simple solution which waits until T2 until the client reacquires the lease by using broadcast; the unicast packets between times T1 and T2 are always lost on these links. Is this something acceptable? Hm, are you sure packets are lost between T1 and T2? These sholud just be regular UDP packets, should they not? Then again, if the network is known to operate correctly, it is better to request and get unicast delivery all the time instead. Should we have a configuration parameter that requests broadcast delivery by default and therefore works in all places? The system owner can then turn on unicast delivery once the network is known to work properly? I'd rather not introduce options to work around bugs, unless there is a really compelling reason... I'd either go with enabling broadcast unconditionally in the cases where it may be necessary, or find a test for deciding whether or not it should be enabled. I now pushed an attempt at a fix. Does it work as expected for everyone? Cheers, Tom ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On Tue, Jun 3, 2014 at 11:37 AM, Tom Gundersen t...@jklm.no wrote: On Mon, Jun 2, 2014 at 2:09 PM, Patrik Flykt patrik.fl...@linux.intel.com wrote: On Fri, 2014-05-30 at 17:21 +0100, Tom Gundersen wrote: I'm wondering if the criterion should be to request broadcast if and only if we have not configured an IP address (I.e. only in discovering, requesting and init-reboot), as that seems to be the problem, or did I get that wrong? Yes, I was aiming at requesting broadcast delivery from the server only for broadcast packets sent by the client. That can be an overly simple solution which waits until T2 until the client reacquires the lease by using broadcast; the unicast packets between times T1 and T2 are always lost on these links. Is this something acceptable? Hm, are you sure packets are lost between T1 and T2? These sholud just be regular UDP packets, should they not? Then again, if the network is known to operate correctly, it is better to request and get unicast delivery all the time instead. Should we have a configuration parameter that requests broadcast delivery by default and therefore works in all places? The system owner can then turn on unicast delivery once the network is known to work properly? I'd rather not introduce options to work around bugs, unless there is a really compelling reason... I'd either go with enabling broadcast unconditionally in the cases where it may be necessary, or find a test for deciding whether or not it should be enabled. I now pushed an attempt at a fix. Does it work as expected for everyone? Correction, I actually pushed precisely the patch that Camilo posted. As the server side is responsible for not sending broadcasts except in the cases I requested in my initial comments, the patch was fine all along. It would be even better if we could detect when to enable broadcasting of course... Cheers, Tom ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On Fri, 2014-05-30 at 17:42 +0100, Tom Gundersen wrote: Actually, I have had recent reports from users that there seems to exist a category of networking devices Do you have any more info on this by any chance? Any way we may classify these devices in a robust way? Unfortunately it was discussed on #connman, so it didn't end up archived. But in the discussion it was pointed out that the issue was the same as described on the ConnMan mailing list in 2013[1]. From that it seems the network boxes are pretty hard to classify, they either let unicast packets through as replies to broadcast ones or then they don't. So still in 2014 there is this not so nice behavior from an unknown set of network devices. Cheers, Patrik [1] https://lists.connman.net/pipermail/connman/2013-February/012891.html ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On Fri, 2014-05-30 at 17:21 +0100, Tom Gundersen wrote: I'm wondering if the criterion should be to request broadcast if and only if we have not configured an IP address (I.e. only in discovering, requesting and init-reboot), as that seems to be the problem, or did I get that wrong? Yes, I was aiming at requesting broadcast delivery from the server only for broadcast packets sent by the client. That can be an overly simple solution which waits until T2 until the client reacquires the lease by using broadcast; the unicast packets between times T1 and T2 are always lost on these links. Is this something acceptable? Then again, if the network is known to operate correctly, it is better to request and get unicast delivery all the time instead. Should we have a configuration parameter that requests broadcast delivery by default and therefore works in all places? The system owner can then turn on unicast delivery once the network is known to work properly? Cheers, Patrik ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
Then again, if the network is known to operate correctly, it is better to request and get unicast delivery all the time instead. Should we have a configuration parameter that requests broadcast delivery by default and therefore works in all places? The system owner can then turn on unicast delivery once the network is known to work properly? What about leaving networkd as it currently is, unicast by default, and just providing the configuration parameter to turn broadcast requests on? It seems like a more reasonable approach for networks with a lot of nodes in the same network segment. If it is done the other way around, it could be taxing good behaving networks. Best, Camilo On Mon, Jun 2, 2014 at 8:09 AM, Patrik Flykt patrik.fl...@linux.intel.com wrote: On Fri, 2014-05-30 at 17:21 +0100, Tom Gundersen wrote: I'm wondering if the criterion should be to request broadcast if and only if we have not configured an IP address (I.e. only in discovering, requesting and init-reboot), as that seems to be the problem, or did I get that wrong? Yes, I was aiming at requesting broadcast delivery from the server only for broadcast packets sent by the client. That can be an overly simple solution which waits until T2 until the client reacquires the lease by using broadcast; the unicast packets between times T1 and T2 are always lost on these links. Is this something acceptable? Then again, if the network is known to operate correctly, it is better to request and get unicast delivery all the time instead. Should we have a configuration parameter that requests broadcast delivery by default and therefore works in all places? The system owner can then turn on unicast delivery once the network is known to work properly? Cheers, Patrik -- *Camilo Aguilar* Software Engineer http://github.com/c4milo ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On Fri, May 30, 2014 at 3:24 AM, Camilo Aguilar camilo.agui...@gmail.com wrote: Hi Tom, the patch did not work because there is no layer2 file. Would it be safe to assume that no file is the same as the file having 0? This is out of my reach unfortunately, but I will be happy to read whatever link you throw at me. I don't think that would work, as only the qeth driver actually exposes the layer2 file (and to the best of my knowledge the other upstream drivers support unicast). If other drivers also can't do unicast, then they need to expose this information to us somehow, I'm afraid that I don't know anything about the VMWare driver, but hopefully we can find a way to get this info out of that driver too (would of course be nice if everyone used the same interface...). Also, to better clarify how ISC dhcp-4.3.0 works. It seems it uses raw sockets as a first attempt (layer2 unicast) and falls back to normal BSD sockets which will always set the broadcast flag on: https://cloudup.com/cAneyImU2H2 and https://gist.github.com/c4milo/017a50a6f5d329887707 I'd really hope we could avoid setting broadcast everywhere, as that seems very wasteful... Cheers, Tom ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On Thu, 2014-05-29 at 00:03 +0100, Tom Gundersen wrote: On Wed, May 28, 2014 at 7:43 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: In systems running on hypervisors this flag needs to be set ON, so offers can reach the virtual machines. For more information please refer to this thread in CoreOS: https://github.com/coreos/bugs/issues/12 Signed-off-by: Camilo Aguilar camilo.agui...@gmail.com --- src/libsystemd-network/sd-dhcp-client.c | 9 + 1 file changed, 9 insertions(+) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 0300a6b..8f54906 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -286,6 +286,15 @@ static int client_message_init(sd_dhcp_client *client, DHCPPacket **ret, refuse to issue an DHCP lease if 'secs' is set to zero */ packet-dhcp.secs = htobe16(client-secs); +/* RFC2132 section 4.1 + A client that cannot receive unicast IP datagrams until its protocol + software has been configured with an IP address SHOULD set the + BROADCAST bit in the 'flags' field to 1 in any DHCPDISCOVER or + DHCPREQUEST messages that client sends. The BROADCAST bit will + provide a hint to the DHCP server and BOOTP relay agent to broadcast + any messages to the client on the client's subnet. */ +packet-dhcp.flags = htobe16(0x8000); Hm, most clients can and should use unicast, so we should definitely not request broadcast unconditionally. If there really is a situation where we need broadcast, we should detect that and only request broadcast in this case. Does that make sense? Actually, I have had recent reports from users that there seems to exist a category of networking devices which discard DHCPv4 messages unless the messages are sent by broadcast. The users say that forcing the DHCPv4 clients to use broadcast fixes the problem. I recently added a patch to ConnMan that always requests broadcast replies when it is sending broadcast itself. That means broadcast DHCPDiscover - DHCPOffer messages as well as DHCPRequest - DHCPAck/Nak messages in Requesting and Rebinding states. Unicast is used for most of the normal message sending in the Renewing state. This way not every message is broadcast and if unicast messages disappear, there is always Rebinding that gets through before lease expiry. Should we do the same thing in networkd? Cheers, Patrik ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On Thu, 2014-05-29 at 18:18 +0100, Tom Gundersen wrote:
+ if (link-udev_device) {
+ const char *l2;
+
+ l2 = udev_device_get_sysattr_value(link-udev_device, layer2);
+ if (l2) {
+ unsigned layer2;
+
+ r = safe_atou(l2, layer2);
+ if (r 0)
+ return r;
+
+ if (!layer2) {
+ r = sd_dhcp_client_request_broadcast(link-dhcp_client);
+ if (r 0)
+ return r;
+ }
Am I now missing something if I can't find the
file /sys/class/net/device/device/layer2 for a regular networking
interface? The above enables broadcast only for virtual interfaces,
right?
Cheers,
Patrik
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
I'd really hope we could avoid setting broadcast everywhere, as that
seems very wasteful...
I agree with you, broadcasting everywhere it's clearly not convenient.
It seems to me the ideal path to move forward looks like this:
1. Use platform specific raw sockets for the first attempt of getting a
DHCPOFFER, through unicast and without ARP. Just like ISC's does
https://kb.isc.org/article/AA-00379/0/How-DHCP-uses-raw-sockets.html.
2. If *1* doesn't work, fall back to normal BSD sockets setting the
broadcast flag on. This is done by ISC's impl too, as I showed in my
previous email. See https://cloudup.com/cAneyImU2H2 and
https://gist.github.com/c4milo/017a50a6f5d329887707
3. Use Tom's patch for qeth devices setting
/sys/class/net/%s/device/layer2 to 0 which seems to happen
https://github.com/bldewolf/dhcpcd/commit/3bc21a1a09852407df89a363c67d7575efa081c8
but it will be nice to confirm.
4. Provide a configuration parameter for administrators to set the
broadcast flag manually.
In essence, Patrik's proposal is the same as 1 and 2, just a different
implementation.
+ if (link-udev_device) {
+ const char *l2;
+
+ l2 = udev_device_get_sysattr_value(link-udev_device, layer2);
+ if (l2) {
+ unsigned layer2;
+
+ r = safe_atou(l2, layer2);
+ if (r 0)
+ return r;
+
+ if (!layer2) {
+ r = sd_dhcp_client_request_
broadcast(link-dhcp_client);
+ if (r 0)
+ return r;
+ }
Am I now missing something if I can't find the
file /sys/class/net/device/device/layer2 for a regular networking
interface? The above enables broadcast only for virtual interfaces,
right?
Patrik, based on what Tom says, I think the above patch will only cover
qeth devices setting /sys/class/net/%s/device/layer2 in 0 which seems to
happen in some cases:
https://github.com/bldewolf/dhcpcd/commit/3bc21a1a09852407df89a363c67d7575efa081c8
The issue that brought me here is consistently happening in VMware virtual
machines using bridged networking: https://github.com/coreos/bugs/issues/12
Best,
Camilo
On Fri, May 30, 2014 at 4:06 AM, Tom Gundersen t...@jklm.no wrote:
On Fri, May 30, 2014 at 3:24 AM, Camilo Aguilar
camilo.agui...@gmail.com wrote:
Hi Tom, the patch did not work because there is no layer2 file. Would it
be
safe to assume that no file is the same as the file having 0? This is
out of
my reach unfortunately, but I will be happy to read whatever link you
throw
at me.
I don't think that would work, as only the qeth driver actually
exposes the layer2 file (and to the best of my knowledge the other
upstream drivers support unicast). If other drivers also can't do
unicast, then they need to expose this information to us somehow, I'm
afraid that I don't know anything about the VMWare driver, but
hopefully we can find a way to get this info out of that driver too
(would of course be nice if everyone used the same interface...).
Also, to better clarify how ISC dhcp-4.3.0 works. It seems it uses raw
sockets as a first attempt (layer2 unicast) and falls back to normal BSD
sockets which will always set the broadcast flag on:
https://cloudup.com/cAneyImU2H2 and
https://gist.github.com/c4milo/017a50a6f5d329887707
I'd really hope we could avoid setting broadcast everywhere, as that
seems very wasteful...
Cheers,
Tom
--
*Camilo Aguilar*
Software Engineer
http://github.com/c4milo
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On 30 May 2014 10:25, Patrik Flykt patrik.fl...@linux.intel.com wrote:
On Thu, 2014-05-29 at 18:18 +0100, Tom Gundersen wrote:
+ if (link-udev_device) {
+ const char *l2;
+
+ l2 = udev_device_get_sysattr_value(link-udev_device, layer2);
+ if (l2) {
+ unsigned layer2;
+
+ r = safe_atou(l2, layer2);
+ if (r 0)
+ return r;
+
+ if (!layer2) {
+ r =
sd_dhcp_client_request_broadcast(link-dhcp_client);
+ if (r 0)
+ return r;
+ }
Am I now missing something if I can't find the
file /sys/class/net/device/device/layer2 for a regular networking
interface? The above enables broadcast only for virtual interfaces,
right?
That's correct. My hope was that the interfaces that require broadcast
would expose that somehow. It appears that's not the case, so a scheme like
yours sounds more robust.
I'm wondering if the criterion should be to request broadcast if and only
if we have not configured an IP address (I.e. only in discovering,
requesting and init-reboot), as that seems to be the problem, or did I get
that wrong?
Cheers,
Tom
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On 30 May 2014 10:12, Patrik Flykt patrik.fl...@linux.intel.com wrote: On Thu, 2014-05-29 at 00:03 +0100, Tom Gundersen wrote: On Wed, May 28, 2014 at 7:43 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: In systems running on hypervisors this flag needs to be set ON, so offers can reach the virtual machines. For more information please refer to this thread in CoreOS: https://github.com/coreos/bugs/issues/12 Signed-off-by: Camilo Aguilar camilo.agui...@gmail.com --- src/libsystemd-network/sd-dhcp-client.c | 9 + 1 file changed, 9 insertions(+) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 0300a6b..8f54906 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -286,6 +286,15 @@ static int client_message_init(sd_dhcp_client *client, DHCPPacket **ret, refuse to issue an DHCP lease if 'secs' is set to zero */ packet-dhcp.secs = htobe16(client-secs); +/* RFC2132 section 4.1 + A client that cannot receive unicast IP datagrams until its protocol + software has been configured with an IP address SHOULD set the + BROADCAST bit in the 'flags' field to 1 in any DHCPDISCOVER or + DHCPREQUEST messages that client sends. The BROADCAST bit will + provide a hint to the DHCP server and BOOTP relay agent to broadcast + any messages to the client on the client's subnet. */ +packet-dhcp.flags = htobe16(0x8000); Hm, most clients can and should use unicast, so we should definitely not request broadcast unconditionally. If there really is a situation where we need broadcast, we should detect that and only request broadcast in this case. Does that make sense? Actually, I have had recent reports from users that there seems to exist a category of networking devices Do you have any more info on this by any chance? Any way we may classify these devices in a robust way? which discard DHCPv4 messages unless the messages are sent by broadcast. The users say that forcing the DHCPv4 clients to use broadcast fixes the problem. I recently added a patch to ConnMan that always requests broadcast replies when it is sending broadcast itself. That means broadcast DHCPDiscover - DHCPOffer messages as well as DHCPRequest - DHCPAck/Nak messages in Requesting and Rebinding states. Unicast is used for most of the normal message sending in the Renewing state. This way not every message is broadcast and if unicast messages disappear, there is always Rebinding that gets through before lease expiry. Should we do the same thing in networkd? Cheers, Patrik ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
gah, the link for 2 is https://github.com/bldewolf/dhcpcd/commit/3bc21a1a09852407df89a363c67d7575efa081c8 instead On Thu, May 29, 2014 at 4:41 AM, Camilo Aguilar camilo.agui...@gmail.com wrote: I spent some time studying the standard ISC's implementation. And, unless I'm misreading the code, for Linux the client seems to always set the broadcast bit ON except when SOCKET_CAN_RECEIVE_UNICAST_UNCONFIGURED is defined in compilation time, which I don't see anybody doing. For the rest of the ports they always sets it OFF. Fedora has a modified version of ISC https://github.com/greearb/dhcp-ct where in addition to the define, they also expose https://github.com/greearb/dhcp-ct/blob/master/dhcp-4.2.0/client/dhclient.c#L2768 setting the broadcast flag through the client's configuration file. The best implementation I found is dhcpcd, used by Gentoo. If I understood well the code, dhcpcd does not send ever the broadcast flag unless two things happen: 1. The administrador runs the client with the --broadcast flag: https://github.com/bldewolf/dhcpcd/commit/5aec6bbd13b0de0c8cc8111d82f827ef7d3eed35 2. The network interface says that it needs broadcast: https://github.com/bldewolf/dhcpcd/commit/fb4477f477d1faa60759f2b94e29c12999e1db49 Any thoughts? Best, Camilo On Wed, May 28, 2014 at 9:39 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: You are right. Apologies. I was stuck on my own work due to this issue and was eager to get it solved too soon. I'll spend some more time tonight digging about how other DHCP clients deal with detecting if the interface supports link level unicast or not. — Sent from Mailbox https://www.dropbox.com/mailbox On Wed, May 28, 2014 at 7:31 PM, Michael Marineau michael.marin...@coreos.com wrote: On Wed, May 28, 2014 at 4:13 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: Oh, never mind, there is no rush since we are already custom patching in CoreOS for now. Hey, don't get ahead of yourself. I haven't merged your patch into CoreOS just yet ;-) I'm fine with the patch being a temporary fix as long as we can sort out the final version soon. On Wed, May 28, 2014 at 7:10 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: It makes total sense. I can provide that patch in addition if you like, But, can we merge this change for now so we can fix https://github.com/coreos/bugs/issues/12 and create a new issue to make it more robust? On Wed, May 28, 2014 at 7:03 PM, Tom Gundersen t...@jklm.no wrote: On Wed, May 28, 2014 at 7:43 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: In systems running on hypervisors this flag needs to be set ON, so offers can reach the virtual machines. For more information please refer to this thread in CoreOS: https://github.com/coreos/bugs/issues/12 Signed-off-by: Camilo Aguilar camilo.agui...@gmail.com --- src/libsystemd-network/sd-dhcp-client.c | 9 + 1 file changed, 9 insertions(+) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 0300a6b..8f54906 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -286,6 +286,15 @@ static int client_message_init(sd_dhcp_client *client, DHCPPacket **ret, refuse to issue an DHCP lease if 'secs' is set to zero */ packet-dhcp.secs = htobe16(client-secs); + /* RFC2132 section 4.1 + A client that cannot receive unicast IP datagrams until its protocol + software has been configured with an IP address SHOULD set the + BROADCAST bit in the 'flags' field to 1 in any DHCPDISCOVER or + DHCPREQUEST messages that client sends. The BROADCAST bit will + provide a hint to the DHCP server and BOOTP relay agent to broadcast + any messages to the client on the client's subnet. */ + packet-dhcp.flags = htobe16(0x8000); Hm, most clients can and should use unicast, so we should definitely not request broadcast unconditionally. If there really is a situation where we need broadcast, we should detect that and only request broadcast in this case. Do you have any ideas on how to detect the issue documented here with VMware virtual machines? I haven't dug into this bug yet myself so I'm not sure what other DHCP implementations are doing off the top of my head. https://github.com/coreos/bugs/issues/12 -- *Camilo Aguilar* Software Engineer http://github.com/c4milo -- *Camilo Aguilar* Software Engineer http://github.com/c4milo ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On Thu, May 29, 2014 at 4:42 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: gah, the link for 2 is https://github.com/bldewolf/dhcpcd/commit/3bc21a1a09852407df89a363c67d7575efa081c8 instead 2. The network interface says that it needs broadcast Do you see that layer2 file in /sys on the machine that does not work? Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
No, I don't see it in any of the machines that are failing. On Thu, May 29, 2014 at 5:04 AM, Kay Sievers k...@vrfy.org wrote: On Thu, May 29, 2014 at 4:42 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: gah, the link for 2 is https://github.com/bldewolf/dhcpcd/commit/3bc21a1a09852407df89a363c67d7575efa081c8 instead 2. The network interface says that it needs broadcast Do you see that layer2 file in /sys on the machine that does not work? Kay -- *Camilo Aguilar* Software Engineer http://github.com/c4milo ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On Thu, May 29, 2014 at 6:05 PM, Camilo Aguilar
camilo.agui...@gmail.com wrote:
No, I don't see it in any of the machines that are failing.
What driver is used on the interface that fails? The qeth driver
should always expose the layer2 value in sysfs as far as I can tell.
The attached (entirely untested) update of your patch should fix the
issue when the layer2 value is available.
Cheers,
Tom
From c2a93db189795abdf69ad6c1bfc1558ba20e5fac Mon Sep 17 00:00:00 2001
From: Camilo Aguilar camilo.agui...@gmail.com
Date: Wed, 28 May 2014 14:43:37 -0400
Subject: [PATCH] sd-dhcp-client: set broadcast flag to 1 when driver does not
support layer2
In systems running on hypervisors this flag needs to be set ON, so offers can reach
the virtual machines.
For more information please refer to this thread in CoreOS: https://github.com/coreos/bugs/issues/12
[tomegun: made the feature conditional]
---
src/libsystemd-network/sd-dhcp-client.c | 21 +
src/network/networkd-link.c | 19 +++
src/systemd/sd-dhcp-client.h| 1 +
3 files changed, 41 insertions(+)
diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c
index 0300a6b..7311b7c 100644
--- a/src/libsystemd-network/sd-dhcp-client.c
+++ b/src/libsystemd-network/sd-dhcp-client.c
@@ -56,6 +56,7 @@ struct sd_dhcp_client {
uint8_t type;
struct ether_addr mac_addr;
} _packed_ client_id;
+bool broadcast;
uint32_t xid;
usec_t start_time;
uint16_t secs;
@@ -180,6 +181,16 @@ int sd_dhcp_client_set_mac(sd_dhcp_client *client,
return 0;
}
+int sd_dhcp_client_request_broadcast(sd_dhcp_client *client) {
+assert_return(client, -EINVAL);
+assert_return(IN_SET(client-state, DHCP_STATE_INIT,
+ DHCP_STATE_STOPPED), -EBUSY);
+
+client-broadcast = true;
+
+return 0;
+}
+
int sd_dhcp_client_get_lease(sd_dhcp_client *client, sd_dhcp_lease **ret) {
assert_return(client, -EINVAL);
assert_return(ret, -EINVAL);
@@ -286,6 +297,16 @@ static int client_message_init(sd_dhcp_client *client, DHCPPacket **ret,
refuse to issue an DHCP lease if 'secs' is set to zero */
packet-dhcp.secs = htobe16(client-secs);
+/* RFC2132 section 4.1
+ A client that cannot receive unicast IP datagrams until its protocol
+ software has been configured with an IP address SHOULD set the
+ BROADCAST bit in the 'flags' field to 1 in any DHCPDISCOVER or
+ DHCPREQUEST messages that client sends. The BROADCAST bit will
+ provide a hint to the DHCP server and BOOTP relay agent to broadcast
+ any messages to the client on the client's subnet. */
+if (client-broadcast)
+packet-dhcp.flags = htobe16(0x8000);
+
/* RFC2132 section 4.1.1:
The client MUST include its hardware address in the ’chaddr’ field, if
necessary for delivery of DHCP reply messages.
diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index 6677b94..b3933ee 100644
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -1640,6 +1640,25 @@ static int link_configure(Link *link) {
if (r 0)
return r;
+if (link-udev_device) {
+const char *l2;
+
+l2 = udev_device_get_sysattr_value(link-udev_device, layer2);
+if (l2) {
+unsigned layer2;
+
+r = safe_atou(l2, layer2);
+if (r 0)
+return r;
+
+if (!layer2) {
+r = sd_dhcp_client_request_broadcast(link-dhcp_client);
+if (r 0)
+return r;
+}
+}
+}
+
r = sd_dhcp_client_attach_event(link-dhcp_client, NULL, 0);
if (r 0)
return r;
diff --git a/src/systemd/sd-dhcp-client.h b/src/systemd/sd-dhcp-client.h
index 5818ec4..e94cdaa 100644
--- a/src/systemd/sd-dhcp-client.h
+++ b/src/systemd/sd-dhcp-client.h
@@ -50,6 +50,7 @@ int sd_dhcp_client_set_request_address(sd_dhcp_client *client,
int sd_dhcp_client_set_index(sd_dhcp_client *client, int interface_index);
int sd_dhcp_client_set_mac(sd_dhcp_client *client,
const struct ether_addr *addr);
+int sd_dhcp_client_request_broadcast(sd_dhcp_client *client);
int sd_dhcp_client_get_lease(sd_dhcp_client *client, sd_dhcp_lease **ret);
int sd_dhcp_client_stop(sd_dhcp_client *client);
--
1.9.0
___
systemd-devel mailing
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
Hi Tom, thanks for the updated patch. I'm going to test it and let you know the results. As for the drivers involved here: e1000 and vmxnet3 (VMware) Best, Camilo On Thu, May 29, 2014 at 1:18 PM, Tom Gundersen t...@jklm.no wrote: On Thu, May 29, 2014 at 6:05 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: No, I don't see it in any of the machines that are failing. What driver is used on the interface that fails? The qeth driver should always expose the layer2 value in sysfs as far as I can tell. The attached (entirely untested) update of your patch should fix the issue when the layer2 value is available. Cheers, Tom -- *Camilo Aguilar* Software Engineer http://github.com/c4milo ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
Hi Tom, the patch did not work because there is no layer2 file. Would it be safe to assume that no file is the same as the file having 0? This is out of my reach unfortunately, but I will be happy to read whatever link you throw at me. Also, to better clarify how ISC dhcp-4.3.0 works. It seems it uses raw sockets as a first attempt (layer2 unicast) and falls back to normal BSD sockets which will always set the broadcast flag on: https://cloudup.com/cAneyImU2H2 and https://gist.github.com/c4milo/017a50a6f5d329887707 Best, Camilo On Thu, May 29, 2014 at 1:41 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: Hi Tom, thanks for the updated patch. I'm going to test it and let you know the results. As for the drivers involved here: e1000 and vmxnet3 (VMware) Best, Camilo On Thu, May 29, 2014 at 1:18 PM, Tom Gundersen t...@jklm.no wrote: On Thu, May 29, 2014 at 6:05 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: No, I don't see it in any of the machines that are failing. What driver is used on the interface that fails? The qeth driver should always expose the layer2 value in sysfs as far as I can tell. The attached (entirely untested) update of your patch should fix the issue when the layer2 value is available. Cheers, Tom -- *Camilo Aguilar* Software Engineer http://github.com/c4milo -- *Camilo Aguilar* Software Engineer http://github.com/c4milo ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
It makes total sense. I can provide that patch in addition if you like, But, can we merge this change for now so we can fix https://github.com/coreos/bugs/issues/12 and create a new issue to make it more robust? On Wed, May 28, 2014 at 7:03 PM, Tom Gundersen t...@jklm.no wrote: On Wed, May 28, 2014 at 7:43 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: In systems running on hypervisors this flag needs to be set ON, so offers can reach the virtual machines. For more information please refer to this thread in CoreOS: https://github.com/coreos/bugs/issues/12 Signed-off-by: Camilo Aguilar camilo.agui...@gmail.com --- src/libsystemd-network/sd-dhcp-client.c | 9 + 1 file changed, 9 insertions(+) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 0300a6b..8f54906 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -286,6 +286,15 @@ static int client_message_init(sd_dhcp_client *client, DHCPPacket **ret, refuse to issue an DHCP lease if 'secs' is set to zero */ packet-dhcp.secs = htobe16(client-secs); +/* RFC2132 section 4.1 + A client that cannot receive unicast IP datagrams until its protocol + software has been configured with an IP address SHOULD set the + BROADCAST bit in the 'flags' field to 1 in any DHCPDISCOVER or + DHCPREQUEST messages that client sends. The BROADCAST bit will + provide a hint to the DHCP server and BOOTP relay agent to broadcast + any messages to the client on the client's subnet. */ +packet-dhcp.flags = htobe16(0x8000); Hm, most clients can and should use unicast, so we should definitely not request broadcast unconditionally. If there really is a situation where we need broadcast, we should detect that and only request broadcast in this case. Does that make sense? Cheers, Tom /* RFC2132 section 4.1.1: The client MUST include its hardware address in the ’chaddr’ field, if necessary for delivery of DHCP reply messages. -- 1.8.5.2 (Apple Git-48) ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel -- *Camilo Aguilar* Software Engineer http://github.com/c4milo ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
Oh, never mind, there is no rush since we are already custom patching in CoreOS for now. On Wed, May 28, 2014 at 7:10 PM, Camilo Aguilar camilo.agui...@gmail.comwrote: It makes total sense. I can provide that patch in addition if you like, But, can we merge this change for now so we can fix https://github.com/coreos/bugs/issues/12 and create a new issue to make it more robust? On Wed, May 28, 2014 at 7:03 PM, Tom Gundersen t...@jklm.no wrote: On Wed, May 28, 2014 at 7:43 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: In systems running on hypervisors this flag needs to be set ON, so offers can reach the virtual machines. For more information please refer to this thread in CoreOS: https://github.com/coreos/bugs/issues/12 Signed-off-by: Camilo Aguilar camilo.agui...@gmail.com --- src/libsystemd-network/sd-dhcp-client.c | 9 + 1 file changed, 9 insertions(+) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 0300a6b..8f54906 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -286,6 +286,15 @@ static int client_message_init(sd_dhcp_client *client, DHCPPacket **ret, refuse to issue an DHCP lease if 'secs' is set to zero */ packet-dhcp.secs = htobe16(client-secs); +/* RFC2132 section 4.1 + A client that cannot receive unicast IP datagrams until its protocol + software has been configured with an IP address SHOULD set the + BROADCAST bit in the 'flags' field to 1 in any DHCPDISCOVER or + DHCPREQUEST messages that client sends. The BROADCAST bit will + provide a hint to the DHCP server and BOOTP relay agent to broadcast + any messages to the client on the client's subnet. */ +packet-dhcp.flags = htobe16(0x8000); Hm, most clients can and should use unicast, so we should definitely not request broadcast unconditionally. If there really is a situation where we need broadcast, we should detect that and only request broadcast in this case. Does that make sense? Cheers, Tom /* RFC2132 section 4.1.1: The client MUST include its hardware address in the ’chaddr’ field, if necessary for delivery of DHCP reply messages. -- 1.8.5.2 (Apple Git-48) ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel -- *Camilo Aguilar* Software Engineer http://github.com/c4milo -- *Camilo Aguilar* Software Engineer http://github.com/c4milo ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
On Wed, May 28, 2014 at 4:13 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: Oh, never mind, there is no rush since we are already custom patching in CoreOS for now. Hey, don't get ahead of yourself. I haven't merged your patch into CoreOS just yet ;-) I'm fine with the patch being a temporary fix as long as we can sort out the final version soon. On Wed, May 28, 2014 at 7:10 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: It makes total sense. I can provide that patch in addition if you like, But, can we merge this change for now so we can fix https://github.com/coreos/bugs/issues/12 and create a new issue to make it more robust? On Wed, May 28, 2014 at 7:03 PM, Tom Gundersen t...@jklm.no wrote: On Wed, May 28, 2014 at 7:43 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: In systems running on hypervisors this flag needs to be set ON, so offers can reach the virtual machines. For more information please refer to this thread in CoreOS: https://github.com/coreos/bugs/issues/12 Signed-off-by: Camilo Aguilar camilo.agui...@gmail.com --- src/libsystemd-network/sd-dhcp-client.c | 9 + 1 file changed, 9 insertions(+) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 0300a6b..8f54906 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -286,6 +286,15 @@ static int client_message_init(sd_dhcp_client *client, DHCPPacket **ret, refuse to issue an DHCP lease if 'secs' is set to zero */ packet-dhcp.secs = htobe16(client-secs); +/* RFC2132 section 4.1 + A client that cannot receive unicast IP datagrams until its protocol + software has been configured with an IP address SHOULD set the + BROADCAST bit in the 'flags' field to 1 in any DHCPDISCOVER or + DHCPREQUEST messages that client sends. The BROADCAST bit will + provide a hint to the DHCP server and BOOTP relay agent to broadcast + any messages to the client on the client's subnet. */ +packet-dhcp.flags = htobe16(0x8000); Hm, most clients can and should use unicast, so we should definitely not request broadcast unconditionally. If there really is a situation where we need broadcast, we should detect that and only request broadcast in this case. Do you have any ideas on how to detect the issue documented here with VMware virtual machines? I haven't dug into this bug yet myself so I'm not sure what other DHCP implementations are doing off the top of my head. https://github.com/coreos/bugs/issues/12 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: Sets broadcast flag to 1
You are right. Apologies. I was stuck on my own work due to this issue and was eager to get it solved too soon. I'll spend some more time tonight digging about how other DHCP clients deal with detecting if the interface supports link level unicast or not. — Sent from Mailbox On Wed, May 28, 2014 at 7:31 PM, Michael Marineau michael.marin...@coreos.com wrote: On Wed, May 28, 2014 at 4:13 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: Oh, never mind, there is no rush since we are already custom patching in CoreOS for now. Hey, don't get ahead of yourself. I haven't merged your patch into CoreOS just yet ;-) I'm fine with the patch being a temporary fix as long as we can sort out the final version soon. On Wed, May 28, 2014 at 7:10 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: It makes total sense. I can provide that patch in addition if you like, But, can we merge this change for now so we can fix https://github.com/coreos/bugs/issues/12 and create a new issue to make it more robust? On Wed, May 28, 2014 at 7:03 PM, Tom Gundersen t...@jklm.no wrote: On Wed, May 28, 2014 at 7:43 PM, Camilo Aguilar camilo.agui...@gmail.com wrote: In systems running on hypervisors this flag needs to be set ON, so offers can reach the virtual machines. For more information please refer to this thread in CoreOS: https://github.com/coreos/bugs/issues/12 Signed-off-by: Camilo Aguilar camilo.agui...@gmail.com --- src/libsystemd-network/sd-dhcp-client.c | 9 + 1 file changed, 9 insertions(+) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 0300a6b..8f54906 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -286,6 +286,15 @@ static int client_message_init(sd_dhcp_client *client, DHCPPacket **ret, refuse to issue an DHCP lease if 'secs' is set to zero */ packet-dhcp.secs = htobe16(client-secs); +/* RFC2132 section 4.1 + A client that cannot receive unicast IP datagrams until its protocol + software has been configured with an IP address SHOULD set the + BROADCAST bit in the 'flags' field to 1 in any DHCPDISCOVER or + DHCPREQUEST messages that client sends. The BROADCAST bit will + provide a hint to the DHCP server and BOOTP relay agent to broadcast + any messages to the client on the client's subnet. */ +packet-dhcp.flags = htobe16(0x8000); Hm, most clients can and should use unicast, so we should definitely not request broadcast unconditionally. If there really is a situation where we need broadcast, we should detect that and only request broadcast in this case. Do you have any ideas on how to detect the issue documented here with VMware virtual machines? I haven't dug into this bug yet myself so I'm not sure what other DHCP implementations are doing off the top of my head. https://github.com/coreos/bugs/issues/12___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
