* [Patch BlueZ 0/1] Make network server interface handling more robust @ 2016-09-18 16:35 Bobby Noelte 2016-09-18 16:35 ` [Patch BlueZ 1/1] " Bobby Noelte 0 siblings, 1 reply; 4+ messages in thread From: Bobby Noelte @ 2016-09-18 16:35 UTC (permalink / raw) To: linux-bluetooth; +Cc: Bobby Noelte Hi, I encountered a lot problems connecting to a NAP server running on Ubuntu 16.04 x86_64. Systemd-udev was renaming the bnepx interfaces before network server could bring up the interface. Network server went on to use the original interface name and failed on the creation of the interface. This did not happen always but quite often. The obvious solution was to prevent udev from renaming. But I could not make that work. Also, IMHO, it is the task of bluetoothd to cope with this situation because systemd-udev is now pretty common. The choosen solution is to use the interface index where possible. It does not change on rename. The kernel does not provide the ifindex on BNEPCONNADD, but the interface name. So there is still a race condition when retrieving the ifindex. Also there are race conditions when using an interface name that was retrieved by interface index; which is necessary for if up/down. To cope with these potential races retry cycles are added. The patched version of bluetoothd was tested using a SAMSUNG S2 (Android 4.1.2) and a NVIDIA Shield (Android 5.1.1). No connection errors due to interface creation were realized so far. The retries are necessary though. The patch is 'minimal invasive'. Only the file bnep.c is changed. Only static local function interfaces of bnep.c are changed/ added. The patch also adds documentation to the static local functions that are substantially changed or added. make check is passed. Comments, suggestions appreciated. Thanks, Bobby b0661n0e71e@gmail.com (1): Make network server interface handling more robust profiles/network/bnep.c | 311 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 260 insertions(+), 51 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Patch BlueZ 1/1] Make network server interface handling more robust 2016-09-18 16:35 [Patch BlueZ 0/1] Make network server interface handling more robust Bobby Noelte @ 2016-09-18 16:35 ` Bobby Noelte 2016-09-19 12:16 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 4+ messages in thread From: Bobby Noelte @ 2016-09-18 16:35 UTC (permalink / raw) To: linux-bluetooth; +Cc: b0661n0e71e@gmail.com From: "b0661n0e71e@gmail.com" <b0661.n0e17e@gmail.com> Interface names may be renamed by udev (systemd-udev) after creation before the interface is up. Interfaces may also be renamed when the interface is down due to a bluetoothd restart. Use the interface index, that does not change on renaming, where possible. Also do some retries when the creation of an interface fails. --- profiles/network/bnep.c | 311 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 260 insertions(+), 51 deletions(-) diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c index 9bf0b18..cba0f63 100644 --- a/profiles/network/bnep.c +++ b/profiles/network/bnep.c @@ -65,6 +65,7 @@ struct bnep { uint16_t dst; bdaddr_t dst_addr; char iface[16]; + unsigned int ifindex; guint attempts; guint setup_to; guint watch; @@ -98,25 +99,109 @@ int bnep_cleanup(void) return 0; } +/** + * private: for local use only. + * + * bnep_indextoname() - Return interface name corresponding to interface index. + * @ifindex interface index. + * @iface interface name, output only, will be filled with '<unknown>' in case of error. + * + * The name is placed in the buffer pointed to by ifname. + * The buffer must allow for the storage of at least IF_NAMESIZE bytes. + * + * On success 0 is returned. On error errno as a negative value is returned. + */ +static int bnep_indextoname(unsigned int ifindex, char *iface) +{ + int err; + + err = 0; + if (if_indextoname(ifindex, iface) == 0) { + err = -errno; + strncpy(iface, "<unknown>", 10); + } + return err; +} + +/** + * private: for local use only. + * + * bnep_nametoindext() - Return interface index corresponding to interface name. + * @iface interface name + * @ifindex interface index, output only, will be filled with 0 in case of error. + * + * The interface index is placed in the variable pointed to by ifindex. + * + * In case the interface is temporarily unavailable do a retry. + * + * On success 0 is returned. On error errno as a negative value is returned. + */ +static int bnep_nametoindex(const char *iface, unsigned int *ifindex) +{ + int retry; + int err; + + retry = 0; + while (retry < 2) { + DBG("get interface index for device %s retry %d", iface, retry); + *ifindex = if_nametoindex(iface); + if (*ifindex != 0) { + DBG("got interface index %d for added device %s retry %d", + *ifindex, iface, retry); + err = 0; + } else { + err = -errno; + } + if (err != -EAGAIN) + break; + /* EAGAIN - Resource temporarily unavailable */ + retry++; + sleep(1); + } + return err; +} + static int bnep_conndel(const bdaddr_t *dst) { + int err; struct bnep_conndel_req req; + char addr[20]; + ba2str(dst, &addr[0]); memset(&req, 0, sizeof(req)); baswap((bdaddr_t *)&req.dst, dst); req.flags = 0; - if (ioctl(ctl, BNEPCONNDEL, &req) < 0) { - int err = -errno; - error("bnep: Failed to kill connection: %s (%d)", - strerror(-err), -err); + err = ioctl(ctl, BNEPCONNDEL, &req); + if (err < 0) { + err = -errno; + error("bnep: Failed to kill connection %s: %s (%d)", + addr, strerror(-err), -err); return err; } + DBG("deleted device %s, %d", addr, err); + return 0; } -static int bnep_connadd(int sk, uint16_t role, char *dev) +/** + * private: for local use only. + * + * bnep_connadd() - Add bluetooth ethernet connection. + * @sk bluez control socket. + * @role + * @dev device name, template for new interface name (e.g. "bnep%d") or name. + * @dst bluetooth address, only needed for retries. + * @ifindex interface index, ouput only - new interface index + * + * In case the interface can not be accessed by the device name (e.g. due to udev rename) do a retry. + * + * On success 0 is returned. On error errno as a negative value is returned. + */ +static int bnep_connadd(int sk, uint16_t role, char *dev, const bdaddr_t *dst, unsigned int *ifindex) { struct bnep_connadd_req req; + int err; + int retry; memset(&req, 0, sizeof(req)); strncpy(req.device, dev, 16); @@ -125,15 +210,40 @@ static int bnep_connadd(int sk, uint16_t role, char *dev) req.sock = sk; req.role = role; req.flags = (1 << BNEP_SETUP_RESPONSE); - if (ioctl(ctl, BNEPCONNADD, &req) < 0) { - int err = -errno; - error("bnep: Failed to add device %s: %s(%d)", - dev, strerror(-err), -err); - return err; + + retry = 0; + + while (retry < 2) { + DBG("add device %s retry %d", req.device, retry); + + err = ioctl(ctl, BNEPCONNADD, &req); + if (err < 0) { + err = -errno; + break; + } + DBG("added device %s retry %d", req.device, retry); + /* + * There is a potential race condition between udev renaming the interface + * after being created by BNEPCONNADD and bnep_nametoindex providing + * the interface index. + */ + err = bnep_nametoindex(&req.device[0], ifindex); + if (!err) + break; + + /* Prepare retry */ + retry++; + bnep_conndel(dst); + /* Give BT some time to really delete. */ + sleep(1); } - strncpy(dev, req.device, 16); - return 0; + if (err) { + error("bnep: Failed to add device %s with interface index %d: %s(%d)", + req.device, *ifindex, strerror(-err), -err); + } + + return err; } static uint32_t bnep_getsuppfeat(void) @@ -148,51 +258,108 @@ static uint32_t bnep_getsuppfeat(void) return feat; } -static int bnep_if_up(const char *devname) +/** + * private: for local use only. + * + * bnep_if_up() - Bring interface up. + * @ifindex interface index + * + * In case the interface can not be accessed (e.g. due to udev rename) do a retry. + * + * On success 0 is returned. On error errno as a negative value is returned. + */ +static int bnep_if_up(unsigned int ifindex) { struct ifreq ifr; - int sk, err = 0; + int sk, err; + int retry; sk = socket(AF_INET, SOCK_DGRAM, 0); memset(&ifr, 0, sizeof(ifr)); - strncpy(ifr.ifr_name, devname, IF_NAMESIZE - 1); - ifr.ifr_flags |= IFF_UP; ifr.ifr_flags |= IFF_MULTICAST; - if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) < 0) { + retry = 0; + while (retry < 2) { + err = bnep_indextoname(ifindex, ifr.ifr_name); + if (err) + break; + /* + * There is a potential race condition between udev renaming the interface + * after the name is retrieved by bnep_nametoindex and the usage of the + * interface name by ioctl. + */ + if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) == 0) + break; err = -errno; - error("bnep: Could not bring up %s: %s(%d)", - devname, strerror(-err), -err); + DBG("could not bring up interface %s with index %d: %s(%d) retry %d", + ifr.ifr_name, ifindex, strerror(-err), -err, retry); + + /* Prepare retry */ + retry++; + sleep(1); } close(sk); + if (err) { + error("bnep: Could not bring up interface %s with index %d: %s(%d)", + ifr.ifr_name, ifindex, strerror(-err), -err); + } + return err; } -static int bnep_if_down(const char *devname) +/** + * private: for local use only. + * + * bnep_if_down() - Bring interface down. + * @ifindex interface index + * + * In case the interface can not be accessed (e.g. due to udev rename) do a retry. + * + * On success 0 is returned. On error errno as a negative value is returned. + */ +static int bnep_if_down(unsigned int ifindex) { struct ifreq ifr; - int sk, err = 0; + int sk, err; + int retry; sk = socket(AF_INET, SOCK_DGRAM, 0); memset(&ifr, 0, sizeof(ifr)); - strncpy(ifr.ifr_name, devname, IF_NAMESIZE - 1); - ifr.ifr_flags &= ~IFF_UP; - /* Bring down the interface */ - if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) < 0) { + retry = 0; + while (retry < 2) { + err = bnep_indextoname(ifindex, ifr.ifr_name); + if (err) + break; + /* + * There is a potential race condition between udev renaming the interface + * after the name is retrieved by bnep_nametoindex and the usage of the + * interface name by ioctl. + */ + if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) == 0) + break; err = -errno; - error("bnep: Could not bring down %s: %s(%d)", - devname, strerror(-err), -err); + DBG("could not bring down interface %s with index %d: %s(%d) retry %d", + ifr.ifr_name, ifindex, strerror(-err), -err, retry); + + /* Prepare retry */ + retry++; + sleep(1); } close(sk); + if (err) { + error("bnep: Could not bring down interface %s with index %d: %s(%d)", + ifr.ifr_name, ifindex, strerror(-err), -err); + } + return err; } @@ -270,14 +437,19 @@ static gboolean bnep_setup_cb(GIOChannel *chan, GIOCondition cond, setsockopt(sk, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo)); sk = g_io_channel_unix_get_fd(session->io); - if (bnep_connadd(sk, session->src, session->iface) < 0) + if (bnep_connadd(sk, session->src, session->iface, &session->dst_addr, &session->ifindex) < 0) goto failed; - if (bnep_if_up(session->iface) < 0) { + if (bnep_if_up(session->ifindex) < 0) { bnep_conndel(&session->dst_addr); goto failed; } + /* Update interface name to current one only after interface is up. + * Only at that time the name should be stable (no udev rename). + */ + bnep_indextoname(session->ifindex, session->iface); + session->watch = g_io_add_watch(session->io, G_IO_ERR | G_IO_HUP | G_IO_NVAL, (GIOFunc) bnep_watchdog_cb, session); @@ -305,7 +477,7 @@ static int bnep_setup_conn_req(struct bnep *session) req = (void *) pkt; req->type = BNEP_CONTROL; req->ctrl = BNEP_SETUP_CONN_REQ; - req->uuid_size = 2; /* 16bit UUID */ + req->uuid_size = 2; /* 16bit UUID */ s = (void *) req->service; s->src = htons(session->src); s->dst = htons(session->dst); @@ -354,6 +526,7 @@ struct bnep *bnep_new(int sk, uint16_t local_role, uint16_t remote_role, session->dst = remote_role; strncpy(session->iface, iface, 16); session->iface[15] = '\0'; + session->ifindex = 0; g_io_channel_set_close_on_unref(session->io, TRUE); session->watch = g_io_add_watch(session->io, @@ -430,20 +603,26 @@ void bnep_disconnect(struct bnep *session) session->io = NULL; } - bnep_if_down(session->iface); + bnep_if_down(session->ifindex); bnep_conndel(&session->dst_addr); } -static int bnep_add_to_bridge(const char *devname, const char *bridge) +static int bnep_add_to_bridge(unsigned int ifindex, const char *bridge) { - int ifindex; struct ifreq ifr; int sk, err = 0; + char devname[IF_NAMESIZE]; - if (!devname || !bridge) + DBG("add interface index %d to bridge %s", ifindex, bridge); + if (!ifindex || !bridge) return -EINVAL; - ifindex = if_nametoindex(devname); + err = bnep_indextoname(ifindex, &devname[0]); + if (err) { + error("bnep: Can't add interface %s with index %d to the bridge %s: %s(%d)", + devname, ifindex, bridge, strerror(-err), -err); + return -EINVAL; + } sk = socket(AF_INET, SOCK_STREAM, 0); if (sk < 0) @@ -451,7 +630,8 @@ static int bnep_add_to_bridge(const char *devname, const char *bridge) memset(&ifr, 0, sizeof(ifr)); strncpy(ifr.ifr_name, bridge, IFNAMSIZ - 1); - ifr.ifr_ifindex = ifindex; + ifr.ifr_ifindex = (int)ifindex; + if (ioctl(sk, SIOCBRADDIF, &ifr) < 0) { err = -errno; @@ -466,16 +646,21 @@ static int bnep_add_to_bridge(const char *devname, const char *bridge) return err; } -static int bnep_del_from_bridge(const char *devname, const char *bridge) +static int bnep_del_from_bridge(unsigned int ifindex, const char *bridge) { - int ifindex; struct ifreq ifr; int sk, err = 0; + char devname[IF_NAMESIZE]; - if (!devname || !bridge) + if (!ifindex || !bridge) return -EINVAL; - ifindex = if_nametoindex(devname); + err = bnep_indextoname(ifindex, &devname[0]); + if (err) { + error("bnep: Can't delete interface %s with index %d from the bridge %s: %s(%d)", + devname, ifindex, bridge, strerror(-err), -err); + return -EINVAL; + } sk = socket(AF_INET, SOCK_STREAM, 0); if (sk < 0) @@ -483,7 +668,7 @@ static int bnep_del_from_bridge(const char *devname, const char *bridge) memset(&ifr, 0, sizeof(ifr)); strncpy(ifr.ifr_name, bridge, IFNAMSIZ - 1); - ifr.ifr_ifindex = ifindex; + ifr.ifr_ifindex = (int)ifindex; if (ioctl(sk, SIOCBRDELIF, &ifr) < 0) { err = -errno; @@ -606,6 +791,7 @@ static int bnep_server_add_legacy(int sk, uint16_t dst, char *bridge, char *iface, const bdaddr_t *addr, uint8_t *setup_data, int len) { + unsigned int ifindex; int err, n; uint16_t rsp; @@ -616,27 +802,32 @@ static int bnep_server_add_legacy(int sk, uint16_t dst, char *bridge, goto reply; } - err = bnep_connadd(sk, dst, iface); + err = bnep_connadd(sk, dst, iface, addr, &ifindex); if (err < 0) { rsp = BNEP_CONN_NOT_ALLOWED; goto reply; } - err = bnep_add_to_bridge(iface, bridge); + err = bnep_add_to_bridge(ifindex, bridge); if (err < 0) { bnep_conndel(addr); rsp = BNEP_CONN_NOT_ALLOWED; goto reply; } - err = bnep_if_up(iface); + err = bnep_if_up(ifindex); if (err < 0) { - bnep_del_from_bridge(iface, bridge); + bnep_del_from_bridge(ifindex, bridge); bnep_conndel(addr); rsp = BNEP_CONN_NOT_ALLOWED; goto reply; } + /* Update interface name to current one only after interface is up. + * Only at that time the name should be stable (no udev rename). + */ + bnep_indextoname(ifindex, iface); + rsp = BNEP_SUCCESS; reply: @@ -652,6 +843,7 @@ reply: int bnep_server_add(int sk, char *bridge, char *iface, const bdaddr_t *addr, uint8_t *setup_data, int len) { + unsigned int ifindex; int err; uint32_t feat; uint16_t rsp, dst; @@ -691,24 +883,29 @@ int bnep_server_add(int sk, char *bridge, char *iface, const bdaddr_t *addr, return bnep_server_add_legacy(sk, dst, bridge, iface, addr, setup_data, len); - err = bnep_connadd(sk, dst, iface); + err = bnep_connadd(sk, dst, iface, addr, &ifindex); if (err < 0) { rsp = BNEP_CONN_NOT_ALLOWED; goto failed; } - err = bnep_add_to_bridge(iface, bridge); + err = bnep_add_to_bridge(ifindex, bridge); if (err < 0) goto failed_conn; - err = bnep_if_up(iface); + err = bnep_if_up(ifindex); if (err < 0) goto failed_bridge; + /* Update interface name to current one only after interface is up. + * Only at that time the name should be stable (no udev rename). + */ + bnep_indextoname(ifindex, iface); + return 0; failed_bridge: - bnep_del_from_bridge(iface, bridge); + bnep_del_from_bridge(ifindex, bridge); failed_conn: bnep_conndel(addr); @@ -727,10 +924,22 @@ failed: void bnep_server_delete(char *bridge, char *iface, const bdaddr_t *addr) { + int err; + unsigned int ifindex; + if (!bridge || !iface || !addr) return; - bnep_del_from_bridge(iface, bridge); - bnep_if_down(iface); + err = bnep_nametoindex(iface, &ifindex); + if (err) { + error("bnep: Failed to delete device %s - interface not available/ renamed: %s(%d)", + iface, strerror(-err), -err); + goto failed; + } + + bnep_del_from_bridge(ifindex, bridge); + bnep_if_down(ifindex); +failed: bnep_conndel(addr); } + -- 2.7.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Patch BlueZ 1/1] Make network server interface handling more robust 2016-09-18 16:35 ` [Patch BlueZ 1/1] " Bobby Noelte @ 2016-09-19 12:16 ` Luiz Augusto von Dentz 2016-09-19 20:28 ` Bernhard Noelte 0 siblings, 1 reply; 4+ messages in thread From: Luiz Augusto von Dentz @ 2016-09-19 12:16 UTC (permalink / raw) To: Bobby Noelte; +Cc: linux-bluetooth@vger.kernel.org, b0661n0e71e@gmail.com Hi Bobby, On Sun, Sep 18, 2016 at 7:35 PM, Bobby Noelte <b0661n0e17e@gmail.com> wrote: > From: "b0661n0e71e@gmail.com" <b0661.n0e17e@gmail.com> > > Interface names may be renamed by udev (systemd-udev) after creation > before the interface is up. Interfaces may also be renamed when > the interface is down due to a bluetoothd restart. I guess there should probably be a way to prevent systemd-udev to change to rename the interface or then we need kernel changes to make sure the interface index is sent. > Use the interface index, that does not change on renaming, where possible. > Also do some retries when the creation of an interface fails. > --- > profiles/network/bnep.c | 311 ++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 260 insertions(+), 51 deletions(-) > > diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c > index 9bf0b18..cba0f63 100644 > --- a/profiles/network/bnep.c > +++ b/profiles/network/bnep.c > @@ -65,6 +65,7 @@ struct bnep { > uint16_t dst; > bdaddr_t dst_addr; > char iface[16]; > + unsigned int ifindex; > guint attempts; > guint setup_to; > guint watch; > @@ -98,25 +99,109 @@ int bnep_cleanup(void) > return 0; > } > > +/** > + * private: for local use only. > + * > + * bnep_indextoname() - Return interface name corresponding to interface index. > + * @ifindex interface index. > + * @iface interface name, output only, will be filled with '<unknown>' in case of error. > + * > + * The name is placed in the buffer pointed to by ifname. > + * The buffer must allow for the storage of at least IF_NAMESIZE bytes. > + * > + * On success 0 is returned. On error errno as a negative value is returned. > + */ > +static int bnep_indextoname(unsigned int ifindex, char *iface) > +{ > + int err; > + > + err = 0; > + if (if_indextoname(ifindex, iface) == 0) { > + err = -errno; > + strncpy(iface, "<unknown>", 10); > + } > + return err; > +} > + > +/** > + * private: for local use only. > + * > + * bnep_nametoindext() - Return interface index corresponding to interface name. > + * @iface interface name > + * @ifindex interface index, output only, will be filled with 0 in case of error. > + * > + * The interface index is placed in the variable pointed to by ifindex. > + * > + * In case the interface is temporarily unavailable do a retry. > + * > + * On success 0 is returned. On error errno as a negative value is returned. > + */ > +static int bnep_nametoindex(const char *iface, unsigned int *ifindex) > +{ > + int retry; > + int err; > + > + retry = 0; > + while (retry < 2) { > + DBG("get interface index for device %s retry %d", iface, retry); > + *ifindex = if_nametoindex(iface); > + if (*ifindex != 0) { > + DBG("got interface index %d for added device %s retry %d", > + *ifindex, iface, retry); > + err = 0; > + } else { > + err = -errno; > + } > + if (err != -EAGAIN) > + break; > + /* EAGAIN - Resource temporarily unavailable */ > + retry++; > + sleep(1); sleep is no acceptable in the daemon code, it should never block in any circumstances. > + } > + return err; > +} > + > static int bnep_conndel(const bdaddr_t *dst) > { > + int err; > struct bnep_conndel_req req; > + char addr[20]; > > + ba2str(dst, &addr[0]); > memset(&req, 0, sizeof(req)); > baswap((bdaddr_t *)&req.dst, dst); > req.flags = 0; > - if (ioctl(ctl, BNEPCONNDEL, &req) < 0) { > - int err = -errno; > - error("bnep: Failed to kill connection: %s (%d)", > - strerror(-err), -err); > + err = ioctl(ctl, BNEPCONNDEL, &req); > + if (err < 0) { > + err = -errno; > + error("bnep: Failed to kill connection %s: %s (%d)", > + addr, strerror(-err), -err); > return err; > } > + DBG("deleted device %s, %d", addr, err); > + > return 0; > } > > -static int bnep_connadd(int sk, uint16_t role, char *dev) > +/** > + * private: for local use only. > + * > + * bnep_connadd() - Add bluetooth ethernet connection. > + * @sk bluez control socket. > + * @role > + * @dev device name, template for new interface name (e.g. "bnep%d") or name. > + * @dst bluetooth address, only needed for retries. > + * @ifindex interface index, ouput only - new interface index > + * > + * In case the interface can not be accessed by the device name (e.g. due to udev rename) do a retry. > + * > + * On success 0 is returned. On error errno as a negative value is returned. > + */ > +static int bnep_connadd(int sk, uint16_t role, char *dev, const bdaddr_t *dst, unsigned int *ifindex) > { > struct bnep_connadd_req req; > + int err; > + int retry; > > memset(&req, 0, sizeof(req)); > strncpy(req.device, dev, 16); > @@ -125,15 +210,40 @@ static int bnep_connadd(int sk, uint16_t role, char *dev) > req.sock = sk; > req.role = role; > req.flags = (1 << BNEP_SETUP_RESPONSE); > - if (ioctl(ctl, BNEPCONNADD, &req) < 0) { > - int err = -errno; > - error("bnep: Failed to add device %s: %s(%d)", > - dev, strerror(-err), -err); > - return err; > + > + retry = 0; > + > + while (retry < 2) { > + DBG("add device %s retry %d", req.device, retry); > + > + err = ioctl(ctl, BNEPCONNADD, &req); > + if (err < 0) { > + err = -errno; > + break; > + } > + DBG("added device %s retry %d", req.device, retry); > + /* > + * There is a potential race condition between udev renaming the interface > + * after being created by BNEPCONNADD and bnep_nametoindex providing > + * the interface index. > + */ > + err = bnep_nametoindex(&req.device[0], ifindex); > + if (!err) > + break; > + > + /* Prepare retry */ > + retry++; > + bnep_conndel(dst); > + /* Give BT some time to really delete. */ > + sleep(1); We aint gonna fix any race condition with sleep. > } > > - strncpy(dev, req.device, 16); > - return 0; > + if (err) { > + error("bnep: Failed to add device %s with interface index %d: %s(%d)", > + req.device, *ifindex, strerror(-err), -err); > + } > + > + return err; > } > > static uint32_t bnep_getsuppfeat(void) > @@ -148,51 +258,108 @@ static uint32_t bnep_getsuppfeat(void) > return feat; > } > > -static int bnep_if_up(const char *devname) > +/** > + * private: for local use only. > + * > + * bnep_if_up() - Bring interface up. > + * @ifindex interface index > + * > + * In case the interface can not be accessed (e.g. due to udev rename) do a retry. > + * > + * On success 0 is returned. On error errno as a negative value is returned. > + */ > +static int bnep_if_up(unsigned int ifindex) > { > struct ifreq ifr; > - int sk, err = 0; > + int sk, err; > + int retry; > > sk = socket(AF_INET, SOCK_DGRAM, 0); > > memset(&ifr, 0, sizeof(ifr)); > - strncpy(ifr.ifr_name, devname, IF_NAMESIZE - 1); > - > ifr.ifr_flags |= IFF_UP; > ifr.ifr_flags |= IFF_MULTICAST; > > - if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) < 0) { > + retry = 0; > + while (retry < 2) { > + err = bnep_indextoname(ifindex, ifr.ifr_name); > + if (err) > + break; > + /* > + * There is a potential race condition between udev renaming the interface > + * after the name is retrieved by bnep_nametoindex and the usage of the > + * interface name by ioctl. > + */ > + if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) == 0) > + break; > err = -errno; > - error("bnep: Could not bring up %s: %s(%d)", > - devname, strerror(-err), -err); > + DBG("could not bring up interface %s with index %d: %s(%d) retry %d", > + ifr.ifr_name, ifindex, strerror(-err), -err, retry); > + > + /* Prepare retry */ > + retry++; > + sleep(1); Ditto. > } > > close(sk); > > + if (err) { > + error("bnep: Could not bring up interface %s with index %d: %s(%d)", > + ifr.ifr_name, ifindex, strerror(-err), -err); > + } > + > return err; > } > > -static int bnep_if_down(const char *devname) > +/** > + * private: for local use only. > + * > + * bnep_if_down() - Bring interface down. > + * @ifindex interface index > + * > + * In case the interface can not be accessed (e.g. due to udev rename) do a retry. > + * > + * On success 0 is returned. On error errno as a negative value is returned. > + */ > +static int bnep_if_down(unsigned int ifindex) > { > struct ifreq ifr; > - int sk, err = 0; > + int sk, err; > + int retry; > > sk = socket(AF_INET, SOCK_DGRAM, 0); > > memset(&ifr, 0, sizeof(ifr)); > - strncpy(ifr.ifr_name, devname, IF_NAMESIZE - 1); > - > ifr.ifr_flags &= ~IFF_UP; > > - /* Bring down the interface */ > - if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) < 0) { > + retry = 0; > + while (retry < 2) { > + err = bnep_indextoname(ifindex, ifr.ifr_name); > + if (err) > + break; > + /* > + * There is a potential race condition between udev renaming the interface > + * after the name is retrieved by bnep_nametoindex and the usage of the > + * interface name by ioctl. > + */ > + if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) == 0) > + break; > err = -errno; > - error("bnep: Could not bring down %s: %s(%d)", > - devname, strerror(-err), -err); > + DBG("could not bring down interface %s with index %d: %s(%d) retry %d", > + ifr.ifr_name, ifindex, strerror(-err), -err, retry); > + > + /* Prepare retry */ > + retry++; > + sleep(1); Ditto. > } > > close(sk); > > + if (err) { > + error("bnep: Could not bring down interface %s with index %d: %s(%d)", > + ifr.ifr_name, ifindex, strerror(-err), -err); > + } > + > return err; > } > > @@ -270,14 +437,19 @@ static gboolean bnep_setup_cb(GIOChannel *chan, GIOCondition cond, > setsockopt(sk, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo)); > > sk = g_io_channel_unix_get_fd(session->io); > - if (bnep_connadd(sk, session->src, session->iface) < 0) > + if (bnep_connadd(sk, session->src, session->iface, &session->dst_addr, &session->ifindex) < 0) > goto failed; > > - if (bnep_if_up(session->iface) < 0) { > + if (bnep_if_up(session->ifindex) < 0) { > bnep_conndel(&session->dst_addr); > goto failed; > } > > + /* Update interface name to current one only after interface is up. > + * Only at that time the name should be stable (no udev rename). > + */ > + bnep_indextoname(session->ifindex, session->iface); > + > session->watch = g_io_add_watch(session->io, > G_IO_ERR | G_IO_HUP | G_IO_NVAL, > (GIOFunc) bnep_watchdog_cb, session); > @@ -305,7 +477,7 @@ static int bnep_setup_conn_req(struct bnep *session) > req = (void *) pkt; > req->type = BNEP_CONTROL; > req->ctrl = BNEP_SETUP_CONN_REQ; > - req->uuid_size = 2; /* 16bit UUID */ > + req->uuid_size = 2; /* 16bit UUID */ > s = (void *) req->service; > s->src = htons(session->src); > s->dst = htons(session->dst); > @@ -354,6 +526,7 @@ struct bnep *bnep_new(int sk, uint16_t local_role, uint16_t remote_role, > session->dst = remote_role; > strncpy(session->iface, iface, 16); > session->iface[15] = '\0'; > + session->ifindex = 0; > > g_io_channel_set_close_on_unref(session->io, TRUE); > session->watch = g_io_add_watch(session->io, > @@ -430,20 +603,26 @@ void bnep_disconnect(struct bnep *session) > session->io = NULL; > } > > - bnep_if_down(session->iface); > + bnep_if_down(session->ifindex); > bnep_conndel(&session->dst_addr); > } > > -static int bnep_add_to_bridge(const char *devname, const char *bridge) > +static int bnep_add_to_bridge(unsigned int ifindex, const char *bridge) > { > - int ifindex; > struct ifreq ifr; > int sk, err = 0; > + char devname[IF_NAMESIZE]; > > - if (!devname || !bridge) > + DBG("add interface index %d to bridge %s", ifindex, bridge); > + if (!ifindex || !bridge) > return -EINVAL; > > - ifindex = if_nametoindex(devname); > + err = bnep_indextoname(ifindex, &devname[0]); > + if (err) { > + error("bnep: Can't add interface %s with index %d to the bridge %s: %s(%d)", > + devname, ifindex, bridge, strerror(-err), -err); > + return -EINVAL; > + } > > sk = socket(AF_INET, SOCK_STREAM, 0); > if (sk < 0) > @@ -451,7 +630,8 @@ static int bnep_add_to_bridge(const char *devname, const char *bridge) > > memset(&ifr, 0, sizeof(ifr)); > strncpy(ifr.ifr_name, bridge, IFNAMSIZ - 1); > - ifr.ifr_ifindex = ifindex; > + ifr.ifr_ifindex = (int)ifindex; > + > > if (ioctl(sk, SIOCBRADDIF, &ifr) < 0) { > err = -errno; > @@ -466,16 +646,21 @@ static int bnep_add_to_bridge(const char *devname, const char *bridge) > return err; > } > > -static int bnep_del_from_bridge(const char *devname, const char *bridge) > +static int bnep_del_from_bridge(unsigned int ifindex, const char *bridge) > { > - int ifindex; > struct ifreq ifr; > int sk, err = 0; > + char devname[IF_NAMESIZE]; > > - if (!devname || !bridge) > + if (!ifindex || !bridge) > return -EINVAL; > > - ifindex = if_nametoindex(devname); > + err = bnep_indextoname(ifindex, &devname[0]); > + if (err) { > + error("bnep: Can't delete interface %s with index %d from the bridge %s: %s(%d)", > + devname, ifindex, bridge, strerror(-err), -err); > + return -EINVAL; > + } > > sk = socket(AF_INET, SOCK_STREAM, 0); > if (sk < 0) > @@ -483,7 +668,7 @@ static int bnep_del_from_bridge(const char *devname, const char *bridge) > > memset(&ifr, 0, sizeof(ifr)); > strncpy(ifr.ifr_name, bridge, IFNAMSIZ - 1); > - ifr.ifr_ifindex = ifindex; > + ifr.ifr_ifindex = (int)ifindex; > > if (ioctl(sk, SIOCBRDELIF, &ifr) < 0) { > err = -errno; > @@ -606,6 +791,7 @@ static int bnep_server_add_legacy(int sk, uint16_t dst, char *bridge, > char *iface, const bdaddr_t *addr, > uint8_t *setup_data, int len) > { > + unsigned int ifindex; > int err, n; > uint16_t rsp; > > @@ -616,27 +802,32 @@ static int bnep_server_add_legacy(int sk, uint16_t dst, char *bridge, > goto reply; > } > > - err = bnep_connadd(sk, dst, iface); > + err = bnep_connadd(sk, dst, iface, addr, &ifindex); > if (err < 0) { > rsp = BNEP_CONN_NOT_ALLOWED; > goto reply; > } > > - err = bnep_add_to_bridge(iface, bridge); > + err = bnep_add_to_bridge(ifindex, bridge); > if (err < 0) { > bnep_conndel(addr); > rsp = BNEP_CONN_NOT_ALLOWED; > goto reply; > } > > - err = bnep_if_up(iface); > + err = bnep_if_up(ifindex); > if (err < 0) { > - bnep_del_from_bridge(iface, bridge); > + bnep_del_from_bridge(ifindex, bridge); > bnep_conndel(addr); > rsp = BNEP_CONN_NOT_ALLOWED; > goto reply; > } > > + /* Update interface name to current one only after interface is up. > + * Only at that time the name should be stable (no udev rename). > + */ > + bnep_indextoname(ifindex, iface); > + > rsp = BNEP_SUCCESS; > > reply: > @@ -652,6 +843,7 @@ reply: > int bnep_server_add(int sk, char *bridge, char *iface, const bdaddr_t *addr, > uint8_t *setup_data, int len) > { > + unsigned int ifindex; > int err; > uint32_t feat; > uint16_t rsp, dst; > @@ -691,24 +883,29 @@ int bnep_server_add(int sk, char *bridge, char *iface, const bdaddr_t *addr, > return bnep_server_add_legacy(sk, dst, bridge, iface, addr, > setup_data, len); > > - err = bnep_connadd(sk, dst, iface); > + err = bnep_connadd(sk, dst, iface, addr, &ifindex); > if (err < 0) { > rsp = BNEP_CONN_NOT_ALLOWED; > goto failed; > } > > - err = bnep_add_to_bridge(iface, bridge); > + err = bnep_add_to_bridge(ifindex, bridge); > if (err < 0) > goto failed_conn; > > - err = bnep_if_up(iface); > + err = bnep_if_up(ifindex); > if (err < 0) > goto failed_bridge; > > + /* Update interface name to current one only after interface is up. > + * Only at that time the name should be stable (no udev rename). > + */ > + bnep_indextoname(ifindex, iface); > + > return 0; > > failed_bridge: > - bnep_del_from_bridge(iface, bridge); > + bnep_del_from_bridge(ifindex, bridge); > > failed_conn: > bnep_conndel(addr); > @@ -727,10 +924,22 @@ failed: > > void bnep_server_delete(char *bridge, char *iface, const bdaddr_t *addr) > { > + int err; > + unsigned int ifindex; > + > if (!bridge || !iface || !addr) > return; > > - bnep_del_from_bridge(iface, bridge); > - bnep_if_down(iface); > + err = bnep_nametoindex(iface, &ifindex); > + if (err) { > + error("bnep: Failed to delete device %s - interface not available/ renamed: %s(%d)", > + iface, strerror(-err), -err); > + goto failed; > + } > + > + bnep_del_from_bridge(ifindex, bridge); > + bnep_if_down(ifindex); > +failed: > bnep_conndel(addr); > } > + > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: [Patch BlueZ 1/1] Make network server interface handling more robust 2016-09-19 12:16 ` Luiz Augusto von Dentz @ 2016-09-19 20:28 ` Bernhard Noelte 0 siblings, 0 replies; 4+ messages in thread From: Bernhard Noelte @ 2016-09-19 20:28 UTC (permalink / raw) To: Luiz Augusto von Dentz, linux-bluetooth Hi Luiz, > I guess there should probably be a way to prevent systemd-udev to > change to rename the interface As I mentioned, I could not manage. Also makes Bluez depend on a configuration where it is not necessary. > or then we need kernel changes to make > sure the interface index is sent. It looks like the "correct" solution, but this one does not change the kernel API. > sleep is no acceptable in the daemon code, it should never block in > any circumstances. Yes, obviously. Thank you for the review. I will prepare a new patch. Bobby Am Montag, 19. September 2016, 15:16:06 CEST schrieben Sie: > Hi Bobby, > > On Sun, Sep 18, 2016 at 7:35 PM, Bobby Noelte <b0661n0e17e@gmail.com> wrote: > > From: "b0661n0e71e@gmail.com" <b0661.n0e17e@gmail.com> > > > > Interface names may be renamed by udev (systemd-udev) after creation > > before the interface is up. Interfaces may also be renamed when > > the interface is down due to a bluetoothd restart. > > I guess there should probably be a way to prevent systemd-udev to > change to rename the interface or then we need kernel changes to make > sure the interface index is sent. > > > Use the interface index, that does not change on renaming, where possible. > > Also do some retries when the creation of an interface fails. > > --- > > > > profiles/network/bnep.c | 311 > > ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 260 > > insertions(+), 51 deletions(-) > > > > diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c > > index 9bf0b18..cba0f63 100644 > > --- a/profiles/network/bnep.c > > +++ b/profiles/network/bnep.c > > @@ -65,6 +65,7 @@ struct bnep { > > > > uint16_t dst; > > bdaddr_t dst_addr; > > char iface[16]; > > > > + unsigned int ifindex; > > > > guint attempts; > > guint setup_to; > > guint watch; > > > > @@ -98,25 +99,109 @@ int bnep_cleanup(void) > > > > return 0; > > > > } > > > > +/** > > + * private: for local use only. > > + * > > + * bnep_indextoname() - Return interface name corresponding to interface > > index. + * @ifindex interface index. > > + * @iface interface name, output only, will be filled with '<unknown>' in > > case of error. + * > > + * The name is placed in the buffer pointed to by ifname. > > + * The buffer must allow for the storage of at least IF_NAMESIZE bytes. > > + * > > + * On success 0 is returned. On error errno as a negative value is > > returned. + */ > > +static int bnep_indextoname(unsigned int ifindex, char *iface) > > +{ > > + int err; > > + > > + err = 0; > > + if (if_indextoname(ifindex, iface) == 0) { > > + err = -errno; > > + strncpy(iface, "<unknown>", 10); > > + } > > + return err; > > +} > > + > > +/** > > + * private: for local use only. > > + * > > + * bnep_nametoindext() - Return interface index corresponding to > > interface name. + * @iface interface name > > + * @ifindex interface index, output only, will be filled with 0 in case > > of error. + * > > + * The interface index is placed in the variable pointed to by ifindex. > > + * > > + * In case the interface is temporarily unavailable do a retry. > > + * > > + * On success 0 is returned. On error errno as a negative value is > > returned. + */ > > +static int bnep_nametoindex(const char *iface, unsigned int *ifindex) > > +{ > > + int retry; > > + int err; > > + > > + retry = 0; > > + while (retry < 2) { > > + DBG("get interface index for device %s retry %d", iface, > > retry); + *ifindex = if_nametoindex(iface); > > + if (*ifindex != 0) { > > + DBG("got interface index %d for added device %s > > retry %d", + *ifindex, iface, retry); > > + err = 0; > > + } else { > > + err = -errno; > > + } > > + if (err != -EAGAIN) > > + break; > > + /* EAGAIN - Resource temporarily unavailable */ > > + retry++; > > + sleep(1); > > sleep is no acceptable in the daemon code, it should never block in > any circumstances. > > > + } > > + return err; > > +} > > + > > > > static int bnep_conndel(const bdaddr_t *dst) > > { > > > > + int err; > > > > struct bnep_conndel_req req; > > > > + char addr[20]; > > > > + ba2str(dst, &addr[0]); > > > > memset(&req, 0, sizeof(req)); > > baswap((bdaddr_t *)&req.dst, dst); > > req.flags = 0; > > > > - if (ioctl(ctl, BNEPCONNDEL, &req) < 0) { > > - int err = -errno; > > - error("bnep: Failed to kill connection: %s (%d)", > > - strerror(-err), > > -err); + err = ioctl(ctl, BNEPCONNDEL, &req); > > + if (err < 0) { > > + err = -errno; > > + error("bnep: Failed to kill connection %s: %s (%d)", > > + addr, strerror(-err), -err); > > > > return err; > > > > } > > > > + DBG("deleted device %s, %d", addr, err); > > + > > > > return 0; > > > > } > > > > -static int bnep_connadd(int sk, uint16_t role, char *dev) > > +/** > > + * private: for local use only. > > + * > > + * bnep_connadd() - Add bluetooth ethernet connection. > > + * @sk bluez control socket. > > + * @role > > + * @dev device name, template for new interface name (e.g. "bnep%d") or > > name. + * @dst bluetooth address, only needed for retries. > > + * @ifindex interface index, ouput only - new interface index > > + * > > + * In case the interface can not be accessed by the device name (e.g. due > > to udev rename) do a retry. + * > > + * On success 0 is returned. On error errno as a negative value is > > returned. + */ > > +static int bnep_connadd(int sk, uint16_t role, char *dev, const bdaddr_t > > *dst, unsigned int *ifindex)> > > { > > > > struct bnep_connadd_req req; > > > > + int err; > > + int retry; > > > > memset(&req, 0, sizeof(req)); > > strncpy(req.device, dev, 16); > > > > @@ -125,15 +210,40 @@ static int bnep_connadd(int sk, uint16_t role, char > > *dev)> > > req.sock = sk; > > req.role = role; > > req.flags = (1 << BNEP_SETUP_RESPONSE); > > > > - if (ioctl(ctl, BNEPCONNADD, &req) < 0) { > > - int err = -errno; > > - error("bnep: Failed to add device %s: %s(%d)", > > - dev, strerror(-err), > > -err); > > - return err; > > + > > + retry = 0; > > + > > + while (retry < 2) { > > + DBG("add device %s retry %d", req.device, retry); > > + > > + err = ioctl(ctl, BNEPCONNADD, &req); > > + if (err < 0) { > > + err = -errno; > > + break; > > + } > > + DBG("added device %s retry %d", req.device, retry); > > + /* > > + * There is a potential race condition between udev > > renaming the interface + * after being created by > > BNEPCONNADD and bnep_nametoindex providing + * the > > interface index. > > + */ > > + err = bnep_nametoindex(&req.device[0], ifindex); > > + if (!err) > > + break; > > + > > + /* Prepare retry */ > > + retry++; > > + bnep_conndel(dst); > > + /* Give BT some time to really delete. */ > > + sleep(1); > > We aint gonna fix any race condition with sleep. > > > } > > > > - strncpy(dev, req.device, 16); > > - return 0; > > + if (err) { > > + error("bnep: Failed to add device %s with interface index > > %d: %s(%d)", + req.device, *ifindex, > > strerror(-err), -err); + } > > + > > + return err; > > > > } > > > > static uint32_t bnep_getsuppfeat(void) > > > > @@ -148,51 +258,108 @@ static uint32_t bnep_getsuppfeat(void) > > > > return feat; > > > > } > > > > -static int bnep_if_up(const char *devname) > > +/** > > + * private: for local use only. > > + * > > + * bnep_if_up() - Bring interface up. > > + * @ifindex interface index > > + * > > + * In case the interface can not be accessed (e.g. due to udev rename) do > > a retry. + * > > + * On success 0 is returned. On error errno as a negative value is > > returned. + */ > > +static int bnep_if_up(unsigned int ifindex) > > > > { > > > > struct ifreq ifr; > > > > - int sk, err = 0; > > + int sk, err; > > + int retry; > > > > sk = socket(AF_INET, SOCK_DGRAM, 0); > > > > memset(&ifr, 0, sizeof(ifr)); > > > > - strncpy(ifr.ifr_name, devname, IF_NAMESIZE - 1); > > - > > > > ifr.ifr_flags |= IFF_UP; > > ifr.ifr_flags |= IFF_MULTICAST; > > > > - if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) < 0) { > > + retry = 0; > > + while (retry < 2) { > > + err = bnep_indextoname(ifindex, ifr.ifr_name); > > + if (err) > > + break; > > + /* > > + * There is a potential race condition between udev > > renaming the interface + * after the name is retrieved by > > bnep_nametoindex and the usage of the + * interface name by > > ioctl. > > + */ > > + if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) == 0) > > + break; > > > > err = -errno; > > > > - error("bnep: Could not bring up %s: %s(%d)", > > - devname, strerror(-err), > > -err); + DBG("could not bring up interface %s with index > > %d: %s(%d) retry %d", + ifr.ifr_name, ifindex, > > strerror(-err), -err, retry); + > > + /* Prepare retry */ > > + retry++; > > + sleep(1); > > Ditto. > > > } > > > > close(sk); > > > > + if (err) { > > + error("bnep: Could not bring up interface %s with index > > %d: %s(%d)", + ifr.ifr_name, ifindex, > > strerror(-err), -err); + } > > + > > > > return err; > > > > } > > > > -static int bnep_if_down(const char *devname) > > +/** > > + * private: for local use only. > > + * > > + * bnep_if_down() - Bring interface down. > > + * @ifindex interface index > > + * > > + * In case the interface can not be accessed (e.g. due to udev rename) do > > a retry. + * > > + * On success 0 is returned. On error errno as a negative value is > > returned. + */ > > +static int bnep_if_down(unsigned int ifindex) > > > > { > > > > struct ifreq ifr; > > > > - int sk, err = 0; > > + int sk, err; > > + int retry; > > > > sk = socket(AF_INET, SOCK_DGRAM, 0); > > > > memset(&ifr, 0, sizeof(ifr)); > > > > - strncpy(ifr.ifr_name, devname, IF_NAMESIZE - 1); > > - > > > > ifr.ifr_flags &= ~IFF_UP; > > > > - /* Bring down the interface */ > > - if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) < 0) { > > + retry = 0; > > + while (retry < 2) { > > + err = bnep_indextoname(ifindex, ifr.ifr_name); > > + if (err) > > + break; > > + /* > > + * There is a potential race condition between udev > > renaming the interface + * after the name is retrieved by > > bnep_nametoindex and the usage of the + * interface name by > > ioctl. > > + */ > > + if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) == 0) > > + break; > > > > err = -errno; > > > > - error("bnep: Could not bring down %s: %s(%d)", > > - devname, strerror(-err), > > -err); + DBG("could not bring down interface %s with index > > %d: %s(%d) retry %d", + ifr.ifr_name, ifindex, > > strerror(-err), -err, retry); + > > + /* Prepare retry */ > > + retry++; > > + sleep(1); > > Ditto. > > > } > > > > close(sk); > > > > + if (err) { > > + error("bnep: Could not bring down interface %s with index > > %d: %s(%d)", + ifr.ifr_name, ifindex, > > strerror(-err), -err); + } > > + > > > > return err; > > > > } > > > > @@ -270,14 +437,19 @@ static gboolean bnep_setup_cb(GIOChannel *chan, > > GIOCondition cond,> > > setsockopt(sk, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo)); > > > > sk = g_io_channel_unix_get_fd(session->io); > > > > - if (bnep_connadd(sk, session->src, session->iface) < 0) > > + if (bnep_connadd(sk, session->src, session->iface, > > &session->dst_addr, &session->ifindex) < 0)> > > goto failed; > > > > - if (bnep_if_up(session->iface) < 0) { > > + if (bnep_if_up(session->ifindex) < 0) { > > > > bnep_conndel(&session->dst_addr); > > goto failed; > > > > } > > > > + /* Update interface name to current one only after interface is > > up. > > + * Only at that time the name should be stable (no udev rename). > > + */ > > + bnep_indextoname(session->ifindex, session->iface); > > + > > > > session->watch = g_io_add_watch(session->io, > > > > G_IO_ERR | G_IO_HUP | G_IO_NVAL, > > (GIOFunc) bnep_watchdog_cb, > > session); > > > > @@ -305,7 +477,7 @@ static int bnep_setup_conn_req(struct bnep *session) > > > > req = (void *) pkt; > > req->type = BNEP_CONTROL; > > req->ctrl = BNEP_SETUP_CONN_REQ; > > > > - req->uuid_size = 2; /* 16bit UUID */ > > + req->uuid_size = 2; /* 16bit UUID */ > > > > s = (void *) req->service; > > s->src = htons(session->src); > > s->dst = htons(session->dst); > > > > @@ -354,6 +526,7 @@ struct bnep *bnep_new(int sk, uint16_t local_role, > > uint16_t remote_role,> > > session->dst = remote_role; > > strncpy(session->iface, iface, 16); > > session->iface[15] = '\0'; > > > > + session->ifindex = 0; > > > > g_io_channel_set_close_on_unref(session->io, TRUE); > > session->watch = g_io_add_watch(session->io, > > > > @@ -430,20 +603,26 @@ void bnep_disconnect(struct bnep *session) > > > > session->io = NULL; > > > > } > > > > - bnep_if_down(session->iface); > > + bnep_if_down(session->ifindex); > > > > bnep_conndel(&session->dst_addr); > > > > } > > > > -static int bnep_add_to_bridge(const char *devname, const char *bridge) > > +static int bnep_add_to_bridge(unsigned int ifindex, const char *bridge) > > > > { > > > > - int ifindex; > > > > struct ifreq ifr; > > int sk, err = 0; > > > > + char devname[IF_NAMESIZE]; > > > > - if (!devname || !bridge) > > + DBG("add interface index %d to bridge %s", ifindex, bridge); > > + if (!ifindex || !bridge) > > > > return -EINVAL; > > > > - ifindex = if_nametoindex(devname); > > + err = bnep_indextoname(ifindex, &devname[0]); > > + if (err) { > > + error("bnep: Can't add interface %s with index %d to the > > bridge %s: %s(%d)", + devname, > > ifindex, bridge, strerror(-err), -err); + return -EINVAL; > > + } > > > > sk = socket(AF_INET, SOCK_STREAM, 0); > > if (sk < 0) > > > > @@ -451,7 +630,8 @@ static int bnep_add_to_bridge(const char *devname, > > const char *bridge)> > > memset(&ifr, 0, sizeof(ifr)); > > strncpy(ifr.ifr_name, bridge, IFNAMSIZ - 1); > > > > - ifr.ifr_ifindex = ifindex; > > + ifr.ifr_ifindex = (int)ifindex; > > + > > > > if (ioctl(sk, SIOCBRADDIF, &ifr) < 0) { > > > > err = -errno; > > > > @@ -466,16 +646,21 @@ static int bnep_add_to_bridge(const char *devname, > > const char *bridge)> > > return err; > > > > } > > > > -static int bnep_del_from_bridge(const char *devname, const char *bridge) > > +static int bnep_del_from_bridge(unsigned int ifindex, const char *bridge) > > > > { > > > > - int ifindex; > > > > struct ifreq ifr; > > int sk, err = 0; > > > > + char devname[IF_NAMESIZE]; > > > > - if (!devname || !bridge) > > + if (!ifindex || !bridge) > > > > return -EINVAL; > > > > - ifindex = if_nametoindex(devname); > > + err = bnep_indextoname(ifindex, &devname[0]); > > + if (err) { > > + error("bnep: Can't delete interface %s with index %d from > > the bridge %s: %s(%d)", + devname, > > ifindex, bridge, strerror(-err), -err); + return -EINVAL; > > + } > > > > sk = socket(AF_INET, SOCK_STREAM, 0); > > if (sk < 0) > > > > @@ -483,7 +668,7 @@ static int bnep_del_from_bridge(const char *devname, > > const char *bridge)> > > memset(&ifr, 0, sizeof(ifr)); > > strncpy(ifr.ifr_name, bridge, IFNAMSIZ - 1); > > > > - ifr.ifr_ifindex = ifindex; > > + ifr.ifr_ifindex = (int)ifindex; > > > > if (ioctl(sk, SIOCBRDELIF, &ifr) < 0) { > > > > err = -errno; > > > > @@ -606,6 +791,7 @@ static int bnep_server_add_legacy(int sk, uint16_t > > dst, char *bridge,> > > char *iface, const bdaddr_t *addr, > > uint8_t *setup_data, int len) > > > > { > > > > + unsigned int ifindex; > > > > int err, n; > > uint16_t rsp; > > > > @@ -616,27 +802,32 @@ static int bnep_server_add_legacy(int sk, uint16_t > > dst, char *bridge,> > > goto reply; > > > > } > > > > - err = bnep_connadd(sk, dst, iface); > > + err = bnep_connadd(sk, dst, iface, addr, &ifindex); > > > > if (err < 0) { > > > > rsp = BNEP_CONN_NOT_ALLOWED; > > goto reply; > > > > } > > > > - err = bnep_add_to_bridge(iface, bridge); > > + err = bnep_add_to_bridge(ifindex, bridge); > > > > if (err < 0) { > > > > bnep_conndel(addr); > > rsp = BNEP_CONN_NOT_ALLOWED; > > goto reply; > > > > } > > > > - err = bnep_if_up(iface); > > + err = bnep_if_up(ifindex); > > > > if (err < 0) { > > > > - bnep_del_from_bridge(iface, bridge); > > + bnep_del_from_bridge(ifindex, bridge); > > > > bnep_conndel(addr); > > rsp = BNEP_CONN_NOT_ALLOWED; > > goto reply; > > > > } > > > > + /* Update interface name to current one only after interface is > > up. > > + * Only at that time the name should be stable (no udev rename). > > + */ > > + bnep_indextoname(ifindex, iface); > > + > > > > rsp = BNEP_SUCCESS; > > > > reply: > > @@ -652,6 +843,7 @@ reply: > > int bnep_server_add(int sk, char *bridge, char *iface, const bdaddr_t > > *addr,> > > uint8_t *setup_data, int > > len) > > > > { > > > > + unsigned int ifindex; > > > > int err; > > uint32_t feat; > > uint16_t rsp, dst; > > > > @@ -691,24 +883,29 @@ int bnep_server_add(int sk, char *bridge, char > > *iface, const bdaddr_t *addr,> > > return bnep_server_add_legacy(sk, dst, bridge, iface, > > addr, > > > > setup_data, len); > > > > - err = bnep_connadd(sk, dst, iface); > > + err = bnep_connadd(sk, dst, iface, addr, &ifindex); > > > > if (err < 0) { > > > > rsp = BNEP_CONN_NOT_ALLOWED; > > goto failed; > > > > } > > > > - err = bnep_add_to_bridge(iface, bridge); > > + err = bnep_add_to_bridge(ifindex, bridge); > > > > if (err < 0) > > > > goto failed_conn; > > > > - err = bnep_if_up(iface); > > + err = bnep_if_up(ifindex); > > > > if (err < 0) > > > > goto failed_bridge; > > > > + /* Update interface name to current one only after interface is > > up. > > + * Only at that time the name should be stable (no udev rename). > > + */ > > + bnep_indextoname(ifindex, iface); > > + > > > > return 0; > > > > failed_bridge: > > - bnep_del_from_bridge(iface, bridge); > > + bnep_del_from_bridge(ifindex, bridge); > > > > failed_conn: > > bnep_conndel(addr); > > > > @@ -727,10 +924,22 @@ failed: > > void bnep_server_delete(char *bridge, char *iface, const bdaddr_t *addr) > > { > > > > + int err; > > + unsigned int ifindex; > > + > > > > if (!bridge || !iface || !addr) > > > > return; > > > > - bnep_del_from_bridge(iface, bridge); > > - bnep_if_down(iface); > > + err = bnep_nametoindex(iface, &ifindex); > > + if (err) { > > + error("bnep: Failed to delete device %s - interface not > > available/ renamed: %s(%d)", + > > iface, strerror(-err), -err); + goto failed; > > + } > > + > > + bnep_del_from_bridge(ifindex, bridge); > > + bnep_if_down(ifindex); > > > > +failed: > > bnep_conndel(addr); > > > > } > > > > + > > -- > > 2.7.4 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" > > in the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-19 20:28 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-18 16:35 [Patch BlueZ 0/1] Make network server interface handling more robust Bobby Noelte 2016-09-18 16:35 ` [Patch BlueZ 1/1] " Bobby Noelte 2016-09-19 12:16 ` Luiz Augusto von Dentz 2016-09-19 20:28 ` Bernhard Noelte
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).