| Message ID | 20200824064630.3836539-1-hegtvedt@cisco.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | ioctl: only memset non-NULL link settings | expand |
On Mon, Aug 24, 2020 at 08:46:30AM +0200, Hans-Christian Noren Egtvedt wrote: > In commit bef780467fa ('ioctl: do not pass transceiver value back to > kernel') a regression slipped. If we have a kernel that does not support > the ETHTOOL_xLINKSETTINGS API, then the do_ioctl_glinksettings() > function will return a NULL pointer. > > Hence before memset'ing the pointer to zero we must first check it is > valid, as NULL return is perfectly fine when running on old kernels. > Please add a standard "Fixes:" line here, i.e. Fixes: bef780467fa7 ("ioctl: do not pass transceiver value back to kernel") > Signed-off-by: Hans-Christian Noren Egtvedt <hegtvedt@cisco.com> > --- > ethtool.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/ethtool.c b/ethtool.c > index c4ad186..8267d6b 100644 > --- a/ethtool.c > +++ b/ethtool.c > @@ -2908,8 +2908,10 @@ static int do_sset(struct cmd_context *ctx) > struct ethtool_link_usettings *link_usettings; > > link_usettings = do_ioctl_glinksettings(ctx); > - memset(&link_usettings->deprecated, 0, > - sizeof(link_usettings->deprecated)); > + if (link_usettings) { > + memset(&link_usettings->deprecated, 0, > + sizeof(link_usettings->deprecated)); > + } > if (link_usettings == NULL) > link_usettings = do_ioctl_gset(ctx); This is correct fix but now we have two if statements with exactly opposite conditions here; please combine them into one if-else. Thank you, Michal > if (link_usettings == NULL) { > -- > 2.25.1 >
On 27/08/2020 10:23, Michal Kubecek wrote: > On Mon, Aug 24, 2020 at 08:46:30AM +0200, Hans-Christian Noren Egtvedt wrote: >> In commit bef780467fa ('ioctl: do not pass transceiver value back to >> kernel') a regression slipped. If we have a kernel that does not support >> the ETHTOOL_xLINKSETTINGS API, then the do_ioctl_glinksettings() >> function will return a NULL pointer. >> >> Hence before memset'ing the pointer to zero we must first check it is >> valid, as NULL return is perfectly fine when running on old kernels. >> > > Please add a standard "Fixes:" line here, i.e. > > Fixes: bef780467fa7 ("ioctl: do not pass transceiver value back to kernel") > >> Signed-off-by: Hans-Christian Noren Egtvedt <hegtvedt@cisco.com> >> --- >> ethtool.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/ethtool.c b/ethtool.c >> index c4ad186..8267d6b 100644 >> --- a/ethtool.c >> +++ b/ethtool.c >> @@ -2908,8 +2908,10 @@ static int do_sset(struct cmd_context *ctx) >> struct ethtool_link_usettings *link_usettings; >> >> link_usettings = do_ioctl_glinksettings(ctx); >> - memset(&link_usettings->deprecated, 0, >> - sizeof(link_usettings->deprecated)); >> + if (link_usettings) { >> + memset(&link_usettings->deprecated, 0, >> + sizeof(link_usettings->deprecated)); >> + } >> if (link_usettings == NULL) >> link_usettings = do_ioctl_gset(ctx); > > This is correct fix but now we have two if statements with exactly > opposite conditions here; please combine them into one if-else. Thank you for the review, a v2 will arrive shortly.
diff --git a/ethtool.c b/ethtool.c index c4ad186..8267d6b 100644 --- a/ethtool.c +++ b/ethtool.c @@ -2908,8 +2908,10 @@ static int do_sset(struct cmd_context *ctx) struct ethtool_link_usettings *link_usettings; link_usettings = do_ioctl_glinksettings(ctx); - memset(&link_usettings->deprecated, 0, - sizeof(link_usettings->deprecated)); + if (link_usettings) { + memset(&link_usettings->deprecated, 0, + sizeof(link_usettings->deprecated)); + } if (link_usettings == NULL) link_usettings = do_ioctl_gset(ctx); if (link_usettings == NULL) {
In commit bef780467fa ('ioctl: do not pass transceiver value back to kernel') a regression slipped. If we have a kernel that does not support the ETHTOOL_xLINKSETTINGS API, then the do_ioctl_glinksettings() function will return a NULL pointer. Hence before memset'ing the pointer to zero we must first check it is valid, as NULL return is perfectly fine when running on old kernels. Signed-off-by: Hans-Christian Noren Egtvedt <hegtvedt@cisco.com> --- ethtool.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)