| Message ID | 1602156697-16669-1-git-send-email-alin.nastac@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [netifd] system-linux: initialize ifreq struct before using it | expand |
Hi Alin, On Thu, Oct 8, 2020 at 1:31 PM Alin Nastac <alin.nastac@gmail.com> wrote: Could you add which issue this fixes ? Thx Hans > > Signed-off-by: Alin Nastac <alin.nastac@gmail.com> > --- > system-linux.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/system-linux.c b/system-linux.c > index 6778b1d..9188899 100644 > --- a/system-linux.c > +++ b/system-linux.c > @@ -904,6 +904,8 @@ failure: > int system_if_resolve(struct device *dev) > { > struct ifreq ifr; > + > + memset(&ifr, 0, sizeof(ifr)); > strncpy(ifr.ifr_name, dev->ifname, sizeof(ifr.ifr_name) - 1); > if (!ioctl(sock_ioctl, SIOCGIFINDEX, &ifr)) > return ifr.ifr_ifindex; > -- > 2.7.4 >
Hi Hans, The issue I have involved adding an external device to the lan bridge through add_device ubus call. Sometimes this operation fails to add the new bridge port with the following device debug traces: Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: device_create_default(525): Create simple device 'wds1_1_2' Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: device_init_virtual(478): Initialize device 'wds1_1_2' Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: device_set_present(634): Network device 'wds1_1_2' is now present Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: __device_add_user(710): Add user for device 'wds1_1_2', refcount=1 Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: device_claim(413): Claim Network device wds1_1_2, new active count: 1 Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: device_claim(432): claim Network device wds1_1_2 failed: -1 I'm not sure it fixes the problem I experience (the jury is still out on that), but it is definitely caused by system_if_resolve() failure to retrieve the interface index. Looking at the handling of SIOCGIFINDEX in the kernel, it shouldn't matter if the rest of the fields are uninitialized, but a variable should always be initialized before its first use. BR, Alin On Thu, Oct 8, 2020 at 2:15 PM Hans Dedecker <dedeckeh@gmail.com> wrote: > > Hi Alin, > > On Thu, Oct 8, 2020 at 1:31 PM Alin Nastac <alin.nastac@gmail.com> wrote: > Could you add which issue this fixes ? > > Thx > Hans > > > > Signed-off-by: Alin Nastac <alin.nastac@gmail.com> > > --- > > system-linux.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/system-linux.c b/system-linux.c > > index 6778b1d..9188899 100644 > > --- a/system-linux.c > > +++ b/system-linux.c > > @@ -904,6 +904,8 @@ failure: > > int system_if_resolve(struct device *dev) > > { > > struct ifreq ifr; > > + > > + memset(&ifr, 0, sizeof(ifr)); > > strncpy(ifr.ifr_name, dev->ifname, sizeof(ifr.ifr_name) - 1); > > if (!ioctl(sock_ioctl, SIOCGIFINDEX, &ifr)) > > return ifr.ifr_ifindex; > > -- > > 2.7.4 > >
Hi Hans, I have the confirmation, this change fixed my problem. Given the fact that my commit fixes an obvious programming error, I don't think I need to put a detailed explanation in the commit description. BR, Alin On Thu, Oct 8, 2020 at 2:35 PM Alin Năstac <alin.nastac@gmail.com> wrote: > > Hi Hans, > > The issue I have involved adding an external device to the lan bridge > through add_device ubus call. Sometimes this operation fails to add > the new bridge port with the following device debug traces: > Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: > device_create_default(525): Create simple device 'wds1_1_2' > Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: > device_init_virtual(478): Initialize device 'wds1_1_2' > Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: > device_set_present(634): Network device 'wds1_1_2' is now present > Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: > __device_add_user(710): Add user for device 'wds1_1_2', refcount=1 > Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: device_claim(413): > Claim Network device wds1_1_2, new active count: 1 > Wed Oct 7 04:22:57 2020 daemon.err netifd[2843]: device_claim(432): > claim Network device wds1_1_2 failed: -1 > > I'm not sure it fixes the problem I experience (the jury is still out > on that), but it is definitely caused by system_if_resolve() failure > to retrieve the interface index. Looking at the handling of > SIOCGIFINDEX in the kernel, it shouldn't matter if the rest of the > fields are uninitialized, but a variable should always be initialized > before its first use. > > BR, > Alin > > On Thu, Oct 8, 2020 at 2:15 PM Hans Dedecker <dedeckeh@gmail.com> wrote: > > > > Hi Alin, > > > > On Thu, Oct 8, 2020 at 1:31 PM Alin Nastac <alin.nastac@gmail.com> wrote: > > Could you add which issue this fixes ? > > > > Thx > > Hans > > > > > > Signed-off-by: Alin Nastac <alin.nastac@gmail.com> > > > --- > > > system-linux.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/system-linux.c b/system-linux.c > > > index 6778b1d..9188899 100644 > > > --- a/system-linux.c > > > +++ b/system-linux.c > > > @@ -904,6 +904,8 @@ failure: > > > int system_if_resolve(struct device *dev) > > > { > > > struct ifreq ifr; > > > + > > > + memset(&ifr, 0, sizeof(ifr)); > > > strncpy(ifr.ifr_name, dev->ifname, sizeof(ifr.ifr_name) - 1); > > > if (!ioctl(sock_ioctl, SIOCGIFINDEX, &ifr)) > > > return ifr.ifr_ifindex; > > > -- > > > 2.7.4 > > >
diff --git a/system-linux.c b/system-linux.c index 6778b1d..9188899 100644 --- a/system-linux.c +++ b/system-linux.c @@ -904,6 +904,8 @@ failure: int system_if_resolve(struct device *dev) { struct ifreq ifr; + + memset(&ifr, 0, sizeof(ifr)); strncpy(ifr.ifr_name, dev->ifname, sizeof(ifr.ifr_name) - 1); if (!ioctl(sock_ioctl, SIOCGIFINDEX, &ifr)) return ifr.ifr_ifindex;
Signed-off-by: Alin Nastac <alin.nastac@gmail.com> --- system-linux.c | 2 ++ 1 file changed, 2 insertions(+)