Hi Marcel, first of all: Whenever you boot from NFS and change your network device address, the system is going to freeze. This goal of this patch is to be able to run ConnMan in an nfsroot (during development) with as little difference to a local boot as possible. There may be additional wireless devices attached which need to get configured by ConnMan and a user interface should be able to query online states and parameters from ConnMan, even though no big changes are allowed to the interface carrying the connection to the NFS server.
On 18.03.2015 17:20, Marcel Holtmann wrote: > Hi Andreas, > >> Booting an nfsroot with connman requires passing -I eth0 to ignore >> the interface. This isn't very nice, for at least the following >> reasons: >> >> * A User interface based on connman is led to believe that there's no >> network interface and thus connman seems to be offline when it's not. >> * The DHCP lease obtained by the kernel won't get renewed. >> * DNS servers won't get obtained from DHCP, thus requiring a workaround >> to copy /proc/net/pnp to /etc/resolv.conf and passing -r to connmand. > > this sentence makes no sense to me. I do not even know what /proc/net/pnp > actually is. When passing ip=dhcp to the kernel, which is common for nfsroot setups, Linux generates a resolv.conf at /proc/net/pnp, just as a reference, without anybody using the nameservers listed in it. So what you get is a network device configured with DHCP but no working nameserver setup. And even if you fix it manually, you have to change ConnMan's behaviour (compared to a boot from local storage) by passing -r to it to disable the DNS proxy or otherwise it will overwrite resolv.conf again, without being able to actually resolve anything (because of -I eth0). > >> Therefore change behaviour to restrict interfaces passed with -I to >> read-only ioctls. >> >> Signed-off-by: Andreas Oberritter <[email protected]> > > No signed-off-by in this project. OK. >> --- >> Makefile.am | 1 + >> include/device.h | 3 ++ >> src/device.c | 34 +++++++++++--- >> src/inet.c | 140 >> ++++++++++++++++++++++++++++++++++++++++++++++++++----- >> src/ipconfig.c | 16 +++++++ >> src/rtnl.c | 20 +------- >> 6 files changed, 178 insertions(+), 36 deletions(-) >> >> diff --git a/Makefile.am b/Makefile.am >> index 19cc8b1..423261f 100644 >> --- a/Makefile.am >> +++ b/Makefile.am >> @@ -210,6 +210,7 @@ src_connmand_CFLAGS = @DBUS_CFLAGS@ @GLIB_CFLAGS@ >> @XTABLES_CFLAGS@ \ >> -DSTORAGEDIR=\""$(storagedir)\"" \ >> -DVPN_STORAGEDIR=\""$(vpn_storagedir)\"" \ >> -DCONFIGDIR=\""$(configdir)\"" \ >> + -DCONNMAND \ > > We are actually not doing build time options. They need to become runtime > options. It's not really an option. Its use is to let inet.c know whether it's getting compiled for connmand or for connman-vpnd. Because both already have different CFLAGS, they won't use the same object file. The rationale was that readonly VPN devices didn't make any sense to me. I can modify the code to pass a parameter around if you prefer, or just allow readonly VPN devices (whatever that may mean). >> -I$(builddir)/src >> >> EXTRA_DIST = src/genbuiltin src/connman-dbus.conf src/connman-polkit.conf \ >> diff --git a/include/device.h b/include/device.h >> index 57b925c..3c9615f 100644 >> --- a/include/device.h >> +++ b/include/device.h >> @@ -93,6 +93,9 @@ int connman_device_set_string(struct connman_device >> *device, >> const char *key, const char *value); >> const char *connman_device_get_string(struct connman_device *device, >> const char *key); >> +void connman_device_set_readonly(struct connman_device *device, >> + bool readonly); >> +bool connman_device_get_readonly(struct connman_device *device); >> >> int connman_device_add_network(struct connman_device *device, >> struct connman_network *network); >> diff --git a/src/device.c b/src/device.c >> index c0683ab..301e850 100644 >> --- a/src/device.c >> +++ b/src/device.c >> @@ -54,6 +54,7 @@ struct connman_device { >> bool powered; >> bool scanning; >> bool disconnected; >> + bool readonly; >> char *name; >> char *node; >> char *address; >> @@ -782,6 +783,32 @@ bool connman_device_get_disconnected(struct >> connman_device *device) >> } >> >> /** >> + * connman_device_set_readonly: >> + * @device: device structure >> + * @readonly: read-only state >> + * >> + * Change read-only state of device >> + */ >> +void connman_device_set_readonly(struct connman_device *device, >> + bool readonly) >> +{ >> + DBG("device %p readonly %d", device, readonly); >> + >> + device->readonly = readonly; >> +} >> + >> +/** >> + * connman_device_get_readonly: >> + * @device: device structure >> + * >> + * Get device read-only state >> + */ >> +bool connman_device_get_readonly(struct connman_device *device) >> +{ >> + return device->readonly; >> +} >> + >> +/** >> * connman_device_set_string: >> * @device: device structure >> * @key: unique identifier >> @@ -1246,12 +1273,6 @@ struct connman_device >> *connman_device_create_from_index(int index) >> if (!devname) >> return NULL; >> >> - if (__connman_device_isfiltered(devname)) { >> - connman_info("Ignoring interface %s (filtered)", devname); >> - g_free(devname); >> - return NULL; >> - } >> - >> type = __connman_rtnl_get_device_type(index); >> >> switch (type) { >> @@ -1305,6 +1326,7 @@ struct connman_device >> *connman_device_create_from_index(int index) >> } >> >> connman_device_set_string(device, "Address", addr); >> + connman_device_set_readonly(device, >> __connman_device_isfiltered(devname)); >> >> done: >> g_free(devname); >> diff --git a/src/inet.c b/src/inet.c >> index cd220ff..ac646c9 100644 >> --- a/src/inet.c >> +++ b/src/inet.c >> @@ -55,6 +55,21 @@ >> ((struct rtattr *) (((uint8_t*) (nmsg)) + \ >> NLMSG_ALIGN((nmsg)->nlmsg_len))) >> >> +static inline int __connman_inet_check_write_perm(int ifindex) >> +{ >> + #if defined(CONNMAND) >> + struct connman_device *dev = connman_device_find_by_index(ifindex); >> + >> + if (!dev) >> + return -ENODEV; >> + >> + if (connman_device_get_readonly(dev)) >> + return -EPERM; >> + #endif >> + >> + return 0; >> +} >> + >> int __connman_inet_rtnl_addattr_l(struct nlmsghdr *n, size_t max_length, >> int type, const void *data, size_t data_length) >> { >> @@ -104,6 +119,11 @@ int __connman_inet_modify_address(int cmd, int flags, >> if (family != AF_INET && family != AF_INET6) >> return -EINVAL; >> >> + if (__connman_inet_check_write_perm(index) < 0) { >> + DBG("insufficient permission, ignoring request"); >> + return 0; >> + } >> + >> memset(&request, 0, sizeof(request)); >> >> header = (struct nlmsghdr *)request; >> @@ -245,6 +265,12 @@ int connman_inet_ifup(int index) >> struct ifreq ifr; >> int sk, err; >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); >> if (sk < 0) >> return -errno; >> @@ -288,6 +314,12 @@ int connman_inet_ifdown(int index) >> struct sockaddr_in *addr; >> int sk, err; >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); >> if (sk < 0) >> return -errno; >> @@ -454,11 +486,17 @@ int connman_inet_add_network_route(int index, const >> char *host, >> struct ifreq ifr; >> struct rtentry rt; >> struct sockaddr_in addr; >> - int sk, err = 0; >> + int sk, err; >> >> DBG("index %d host %s gateway %s netmask %s", index, >> host, gateway, netmask); >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); >> if (sk < 0) { >> err = -errno; >> @@ -525,10 +563,16 @@ int connman_inet_del_network_route(int index, const >> char *host) >> struct ifreq ifr; >> struct rtentry rt; >> struct sockaddr_in addr; >> - int sk, err = 0; >> + int sk, err; >> >> DBG("index %d host %s", index, host); >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); >> if (sk < 0) { >> err = -errno; >> @@ -573,13 +617,19 @@ int connman_inet_del_ipv6_network_route(int index, >> const char *host, >> unsigned char prefix_len) >> { >> struct in6_rtmsg rt; >> - int sk, err = 0; >> + int sk, err; >> >> DBG("index %d host %s", index, host); >> >> if (!host) >> return -EINVAL; >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> memset(&rt, 0, sizeof(rt)); >> >> rt.rtmsg_dst_len = prefix_len; >> @@ -623,13 +673,19 @@ int connman_inet_add_ipv6_network_route(int index, >> const char *host, >> unsigned char prefix_len) >> { >> struct in6_rtmsg rt; >> - int sk, err = 0; >> + int sk, err; >> >> DBG("index %d host %s gateway %s", index, host, gateway); >> >> if (!host) >> return -EINVAL; >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> memset(&rt, 0, sizeof(rt)); >> >> rt.rtmsg_dst_len = prefix_len; >> @@ -677,13 +733,19 @@ int connman_inet_add_ipv6_host_route(int index, const >> char *host, >> int connman_inet_clear_ipv6_gateway_address(int index, const char *gateway) >> { >> struct in6_rtmsg rt; >> - int sk, err = 0; >> + int sk, err; >> >> DBG("index %d gateway %s", index, gateway); >> >> if (!gateway) >> return -EINVAL; >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> memset(&rt, 0, sizeof(rt)); >> >> if (inet_pton(AF_INET6, gateway, &rt.rtmsg_gateway) < 0) { >> @@ -720,10 +782,16 @@ int connman_inet_set_gateway_interface(int index) >> struct ifreq ifr; >> struct rtentry rt; >> struct sockaddr_in addr; >> - int sk, err = 0; >> + int sk, err; >> >> DBG("index %d", index); >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); >> if (sk < 0) { >> err = -errno; >> @@ -773,10 +841,16 @@ int connman_inet_set_ipv6_gateway_interface(int index) >> struct rtentry rt; >> struct sockaddr_in6 addr; >> const struct in6_addr any = IN6ADDR_ANY_INIT; >> - int sk, err = 0; >> + int sk, err; >> >> DBG("index %d", index); >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(PF_INET6, SOCK_DGRAM | SOCK_CLOEXEC, 0); >> if (sk < 0) { >> err = -errno; >> @@ -825,10 +899,16 @@ int connman_inet_clear_gateway_address(int index, >> const char *gateway) >> struct ifreq ifr; >> struct rtentry rt; >> struct sockaddr_in addr; >> - int sk, err = 0; >> + int sk, err; >> >> DBG("index %d gateway %s", index, gateway); >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); >> if (sk < 0) { >> err = -errno; >> @@ -882,10 +962,16 @@ int connman_inet_clear_gateway_interface(int index) >> struct ifreq ifr; >> struct rtentry rt; >> struct sockaddr_in addr; >> - int sk, err = 0; >> + int sk, err; >> >> DBG("index %d", index); >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); >> if (sk < 0) { >> err = -errno; >> @@ -935,10 +1021,16 @@ int connman_inet_clear_ipv6_gateway_interface(int >> index) >> struct rtentry rt; >> struct sockaddr_in6 addr; >> const struct in6_addr any = IN6ADDR_ANY_INIT; >> - int sk, err = 0; >> + int sk, err; >> >> DBG("index %d", index); >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(PF_INET6, SOCK_DGRAM | SOCK_CLOEXEC, 0); >> if (sk < 0) { >> err = -errno; >> @@ -1035,11 +1127,17 @@ bool connman_inet_compare_subnet(int index, const >> char *host) >> int connman_inet_remove_from_bridge(int index, const char *bridge) >> { >> struct ifreq ifr; >> - int sk, err = 0; >> + int sk, err; >> >> if (!bridge) >> return -EINVAL; >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); >> if (sk < 0) { >> err = -errno; >> @@ -1066,11 +1164,17 @@ out: >> int connman_inet_add_to_bridge(int index, const char *bridge) >> { >> struct ifreq ifr; >> - int sk, err = 0; >> + int sk, err; >> >> if (!bridge) >> return -EINVAL; >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); >> if (sk < 0) { >> err = -errno; >> @@ -1099,6 +1203,12 @@ int connman_inet_set_mtu(int index, int mtu) >> struct ifreq ifr; >> int sk, err; >> >> + err = __connman_inet_check_write_perm(index); >> + if (err < 0) { >> + DBG("insufficient permission"); >> + return err; >> + } >> + >> sk = socket(AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); >> if (sk < 0) >> return sk; >> @@ -2812,6 +2922,12 @@ static int iproute_default_modify(int cmd, uint32_t >> table_id, int ifindex, >> if (ret <= 0) >> return -EINVAL; >> >> + ret = __connman_inet_check_write_perm(ifindex); >> + if (ret < 0) { >> + DBG("insufficient permission"); >> + return ret; >> + } >> + >> memset(&rth, 0, sizeof(rth)); >> >> rth.req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)); >> diff --git a/src/ipconfig.c b/src/ipconfig.c >> index f8c148b..3ecabfc 100644 >> --- a/src/ipconfig.c >> +++ b/src/ipconfig.c >> @@ -90,6 +90,17 @@ static GHashTable *ipdevice_hash = NULL; >> static GList *ipconfig_list = NULL; >> static bool is_ipv6_supported = false; >> >> +static int __connman_ipconfig_check_write_perm(const gchar *ifname) >> +{ >> + int index = connman_inet_ifindex(ifname); >> + struct connman_device *dev = connman_device_find_by_index(index); >> + >> + if (!dev) >> + return -ENODEV; >> + >> + return connman_device_get_readonly(dev) ? -EPERM : 0; >> +} >> + >> void __connman_ipconfig_clear_address(struct connman_ipconfig *ipconfig) >> { >> if (!ipconfig) >> @@ -210,6 +221,8 @@ static void set_ipv6_state(gchar *ifname, bool enable) >> >> if (!ifname) >> path = g_strdup("/proc/sys/net/ipv6/conf/all/disable_ipv6"); >> + else if (__connman_ipconfig_check_write_perm(ifname) < 0) >> + return; >> else >> path = g_strdup_printf( >> "/proc/sys/net/ipv6/conf/%s/disable_ipv6", ifname); >> @@ -273,6 +286,9 @@ static void set_ipv6_privacy(gchar *ifname, int value) >> if (!ifname) >> return; >> >> + if (__connman_ipconfig_check_write_perm(ifname) < 0) >> + return; >> + >> path = g_strdup_printf("/proc/sys/net/ipv6/conf/%s/use_tempaddr", >> ifname); >> >> diff --git a/src/rtnl.c b/src/rtnl.c >> index b8b02c4..25da07b 100644 >> --- a/src/rtnl.c >> +++ b/src/rtnl.c >> @@ -83,17 +83,6 @@ static void free_interface(gpointer data) >> g_free(interface); >> } >> >> -static bool ether_blacklisted(const char *name) >> -{ >> - if (!name) >> - return true; >> - >> - if (__connman_device_isfiltered(name)) >> - return true; >> - >> - return false; >> -} >> - > > You are taking existing functionality out. That is not really possible and > might break existing users. It's static, so there are no existing users left after the following hunk is applied. > >> static bool wext_interface(char *ifname) >> { >> struct iwreq wrq; >> @@ -124,13 +113,8 @@ static void read_uevent(struct interface_data >> *interface) >> >> name = connman_inet_ifname(interface->index); >> >> - if (ether_blacklisted(name)) { >> - interface->service_type = CONNMAN_SERVICE_TYPE_UNKNOWN; >> - interface->device_type = CONNMAN_DEVICE_TYPE_UNKNOWN; >> - } else { >> - interface->service_type = CONNMAN_SERVICE_TYPE_ETHERNET; >> - interface->device_type = CONNMAN_DEVICE_TYPE_ETHERNET; >> - } >> + interface->service_type = CONNMAN_SERVICE_TYPE_ETHERNET; >> + interface->device_type = CONNMAN_DEVICE_TYPE_ETHERNET; >> >> filename = g_strdup_printf("/sys/class/net/%s/uevent", name); > > How is this making it better. It now gets classified as Ethernet, but ConnMan > can not do anything with that interface? Who is running DHCP etc. now? ConnMan can provide all device properties to its clients. ConnMan can also renew DHCP leases, right? Of course it can't change the address the kernel set at boot, but that's what you need on nfsroots. Regards, Andreas > > Regards > > Marcel > > _______________________________________________ > connman mailing list > [email protected] > https://lists.connman.net/mailman/listinfo/connman > _______________________________________________ connman mailing list [email protected] https://lists.connman.net/mailman/listinfo/connman
