Re: [PATCH 12/13] ap: add support for DHCPv4 server

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6442 bytes --]

Hi James,

On 10/20/20 1:02 PM, James Prestwood wrote:
> The DHCP server can be enabled by including an [IPv4] group
> in the AP provisioning file and including at least a subnet
> mask.

Any chance we can add this code to the current implementation without changing 
the API just yet?  I think it is safe to assume that if the address is set prior 
to AP starting on a given interface, then we should not start our own DHCP, and 
if the address isn't set, then start DHCP server.  If the address is provided by 
provisioning, then start our own DHCP as well, overriding whatever we picked.

> ---
>   src/ap.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>   src/ap.h |   8 +++++
>   2 files changed, 110 insertions(+), 6 deletions(-)
> 
> diff --git a/src/ap.c b/src/ap.c
> index 41de69a9..e9f79e5a 100644
> --- a/src/ap.c
> +++ b/src/ap.c
> @@ -76,6 +76,8 @@ struct ap_state {
>   	uint16_t last_aid;
>   	struct l_queue *sta_states;
>   
> +	struct l_dhcp_server *server;
> +
>   	bool started : 1;
>   	bool gtk_set : 1;
>   };
> @@ -120,6 +122,13 @@ void ap_config_free(struct ap_config *config)
>   	explicit_bzero(config->psk, sizeof(config->psk));
>   	l_free(config->authorized_macs);
>   	l_free(config->wsc_name);
> +
> +	l_free(config->address);
> +	l_free(config->gateway);
> +	l_free(config->netmask);
> +	l_strv_free(config->dns_list);
> +	l_strv_free(config->ip_range);
> +
>   	l_free(config);
>   }
>   
> @@ -198,6 +207,9 @@ static void ap_reset(struct ap_state *ap)
>   	l_queue_destroy(ap->wsc_pbc_probes, l_free);
>   
>   	ap->started = false;
> +
> +	if (ap->server)
> +		l_dhcp_server_stop(ap->server);
>   }
>   
>   static void ap_del_station(struct sta_state *sta, uint16_t reason,
> @@ -1906,6 +1918,55 @@ static void ap_deauth_cb(const struct mmpdu_header *hdr, const void *body,
>   	ap_sta_free(sta);
>   }
>   
> +static bool ap_start_dhcp(struct ap_state *ap)
> +{
> +	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
> +
> +	/*
> +	 * [IPv4].Netmask is the only required property since .Address and
> +	 * .Gateway may inherit from the interfaces IP that was already set.
> +	 * If this is not found we assume DHCP is not being used.

Netmask is also inherited actually.  Its the prefix len...

denkenz(a)localhost ~/iwd-master $ ip addr
...
inet 192.168.1.249/24 brd 192.168.1.255 scope global dynamic noprefixroute
...

/24 = 255.255.255.0

> +	 */
> +	if (!ap->config->netmask)
> +		return true;
> +
> +	ap->server = l_dhcp_server_new(ifindex);
> +	if (!ap->server) {
> +		l_error("Failed to create DHCP server on %u", ifindex);
> +		return false;
> +	}
> +
> +	/* TODO: set address via rtnl */
> +	if (ap->config->address)
> +		if (!l_dhcp_server_set_ip_address(ap->server,
> +							ap->config->address))
> +			return false;
> +
> +	if (ap->config->gateway)
> +		if (!l_dhcp_server_set_gateway(ap->server, ap->config->gateway))
> +			return false;
> +
> +	if (ap->config->dns_list)
> +		if (!l_dhcp_server_set_dns(ap->server, ap->config->dns_list))
> +			return false;
> +
> +	if (ap->config->lease_time)
> +		if (!l_dhcp_server_set_lease_time(ap->server,
> +						ap->config->lease_time))
> +			return false;
> +
> +	if (ap->config->ip_range)
> +		if (!l_dhcp_server_set_ip_range(ap->server,
> +						ap->config->ip_range[0],
> +						ap->config->ip_range[1]))
> +			return false;
> +
> +	if (!l_dhcp_server_set_netmask(ap->server, ap->config->netmask))
> +		return false;
> +
> +	return l_dhcp_server_start(ap->server);
> +}
> +
>   static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
>   {
>   	struct ap_state *ap = user_data;
> @@ -1915,16 +1976,24 @@ static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
>   	if (l_genl_msg_get_error(msg) < 0) {
>   		l_error("START_AP failed: %i", l_genl_msg_get_error(msg));
>   
> -		ap->ops->handle_event(AP_EVENT_START_FAILED, NULL,
> -					ap->user_data);
> -		ap_reset(ap);
> -		l_genl_family_free(ap->nl80211);
> -		l_free(ap);
> -		return;
> +		goto failed;
> +	}
> +
> +	if (!ap_start_dhcp(ap)) {
> +		l_error("DHCP server failed to start");
> +		goto failed;
>   	}
>   
>   	ap->started = true;
>   	ap->ops->handle_event(AP_EVENT_STARTED, NULL, ap->user_data);
> +
> +	return;
> +
> +failed:
> +	ap->ops->handle_event(AP_EVENT_START_FAILED, NULL, ap->user_data);
> +	ap_reset(ap);
> +	l_genl_family_free(ap->nl80211);
> +	l_free(ap);
>   }
>   
>   static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
> @@ -2563,6 +2632,33 @@ static struct ap_config *ap_config_from_settings(struct l_settings *settings,
>   		l_free(passphrase);
>   	}
>   
> +	if (!l_settings_has_group(settings, "IPv4"))
> +		goto done;
> +
> +	config->address = l_settings_get_string(settings, "IPv4", "Address");
> +	config->gateway = l_settings_get_string(settings, "IPv4", "Gateway");
> +	config->netmask = l_settings_get_string(settings, "IPv4", "Netmask");
> +	config->dns_list = l_settings_get_string_list(settings, "IPv4",
> +							"DNSList", ',');
> +	config->ip_range = l_settings_get_string_list(settings, "IPv4",
> +							"IPRange", ',');
> +
> +	if (!l_settings_get_uint(settings, "IPv4", "LeaseTime",
> +					&config->lease_time))
> +		config->lease_time = 0;
> +
> +
> +	if (!config->netmask) {
> +		l_error("[IPv4].Netmask is a required value for DHCP");
> +		goto failed;
> +	}
> +
> +	if (config->ip_range && l_strv_length(config->ip_range) != 2) {
> +		l_error("IP range must be 2 values");
> +		goto failed;
> +	}
> +
> +
>   done:
>   	l_info("Loaded AP configuration %s", config->ssid);
>   	return config;
> diff --git a/src/ap.h b/src/ap.h
> index 90497008..277a5a2f 100644
> --- a/src/ap.h
> +++ b/src/ap.h
> @@ -63,6 +63,14 @@ struct ap_config {
>   	unsigned int authorized_macs_num;
>   	char *wsc_name;
>   	struct wsc_primary_device_type wsc_primary_device_type;
> +
> +	char *address;
> +	char *gateway;
> +	char *netmask;
> +	char **dns_list;
> +	char **ip_range;
> +	uint32_t lease_time;
> +

I think we may need Andrew's opinion on whether these should be part of 
ap_config.  P2P might not really care about setting up these variables and would 
prefer for ap.c to just figure things out.

>   	bool no_cck_rates : 1;
>   	bool no_free : 1;
>   };
> 

Regards,
-Denis