* [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte @ 2019-09-03 1:08 Gustavo A. R. Silva 2019-09-03 1:20 ` Vladimir Oltean 2019-09-03 7:19 ` Eric Dumazet 0 siblings, 2 replies; 7+ messages in thread From: Gustavo A. R. Silva @ 2019-09-03 1:08 UTC (permalink / raw) To: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Vladimir Oltean Cc: netdev, linux-kernel, Gustavo A. R. Silva Add suffix LL to constant 1000 in order to avoid a potential integer overflow and give the compiler complete information about the proper arithmetic to use. Notice that this constant is being used in a context that expects an expression of type s64, but it's currently evaluated using 32-bit arithmetic. Addresses-Coverity-ID: 1453459 ("Unintentional integer overflow") Fixes: f04b514c0ce2 ("taprio: Set default link speed to 10 Mbps in taprio_set_picos_per_byte") Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- net/sched/sch_taprio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index 8d8bc2ec5cd6..956f837436ea 100644 --- a/net/sched/sch_taprio.c +++ b/net/sched/sch_taprio.c @@ -966,7 +966,7 @@ static void taprio_set_picos_per_byte(struct net_device *dev, skip: picos_per_byte = div64_s64(NSEC_PER_SEC * 1000LL * 8, - speed * 1000 * 1000); + speed * 1000LL * 1000); atomic64_set(&q->picos_per_byte, picos_per_byte); netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %lld, linkspeed: %d\n", -- 2.23.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte 2019-09-03 1:08 [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte Gustavo A. R. Silva @ 2019-09-03 1:20 ` Vladimir Oltean 2019-09-03 7:19 ` Eric Dumazet 1 sibling, 0 replies; 7+ messages in thread From: Vladimir Oltean @ 2019-09-03 1:20 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, netdev, lkml, Vinicius Costa Gomes On Tue, 3 Sep 2019 at 04:08, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote: > > Add suffix LL to constant 1000 in order to avoid a potential integer > overflow and give the compiler complete information about the proper > arithmetic to use. Notice that this constant is being used in a context > that expects an expression of type s64, but it's currently evaluated > using 32-bit arithmetic. > > Addresses-Coverity-ID: 1453459 ("Unintentional integer overflow") > Fixes: f04b514c0ce2 ("taprio: Set default link speed to 10 Mbps in taprio_set_picos_per_byte") > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- Acked-by: Vladimir Oltean <olteanv@gmail.com> > net/sched/sch_taprio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c > index 8d8bc2ec5cd6..956f837436ea 100644 > --- a/net/sched/sch_taprio.c > +++ b/net/sched/sch_taprio.c > @@ -966,7 +966,7 @@ static void taprio_set_picos_per_byte(struct net_device *dev, > > skip: > picos_per_byte = div64_s64(NSEC_PER_SEC * 1000LL * 8, > - speed * 1000 * 1000); > + speed * 1000LL * 1000); > > atomic64_set(&q->picos_per_byte, picos_per_byte); > netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %lld, linkspeed: %d\n", > -- > 2.23.0 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte 2019-09-03 1:08 [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte Gustavo A. R. Silva 2019-09-03 1:20 ` Vladimir Oltean @ 2019-09-03 7:19 ` Eric Dumazet 2019-09-03 10:12 ` Vladimir Oltean 1 sibling, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2019-09-03 7:19 UTC (permalink / raw) To: Gustavo A. R. Silva, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Vladimir Oltean Cc: netdev, linux-kernel On 9/3/19 3:08 AM, Gustavo A. R. Silva wrote: > Add suffix LL to constant 1000 in order to avoid a potential integer > overflow and give the compiler complete information about the proper > arithmetic to use. Notice that this constant is being used in a context > that expects an expression of type s64, but it's currently evaluated > using 32-bit arithmetic. > > Addresses-Coverity-ID: 1453459 ("Unintentional integer overflow") > Fixes: f04b514c0ce2 ("taprio: Set default link speed to 10 Mbps in taprio_set_picos_per_byte") > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > net/sched/sch_taprio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c > index 8d8bc2ec5cd6..956f837436ea 100644 > --- a/net/sched/sch_taprio.c > +++ b/net/sched/sch_taprio.c > @@ -966,7 +966,7 @@ static void taprio_set_picos_per_byte(struct net_device *dev, > > skip: > picos_per_byte = div64_s64(NSEC_PER_SEC * 1000LL * 8, > - speed * 1000 * 1000); > + speed * 1000LL * 1000); > > atomic64_set(&q->picos_per_byte, picos_per_byte); > netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %lld, linkspeed: %d\n", > But, why even multiplying by 1,000,000 in the first place, this seems silly, a standard 32 bit divide could be used instead. -> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index 8d8bc2ec5cd6281d811fd5d8a5c5211ebb0edd73..944b1af3215668e927d486b6c6c65c4599fb9539 100644 --- a/net/sched/sch_taprio.c +++ b/net/sched/sch_taprio.c @@ -965,8 +965,7 @@ static void taprio_set_picos_per_byte(struct net_device *dev, speed = ecmd.base.speed; skip: - picos_per_byte = div64_s64(NSEC_PER_SEC * 1000LL * 8, - speed * 1000 * 1000); + picos_per_byte = (USEC_PER_SEC * 8) / speed; atomic64_set(&q->picos_per_byte, picos_per_byte); netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %lld, linkspeed: %d\n", ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte 2019-09-03 7:19 ` Eric Dumazet @ 2019-09-03 10:12 ` Vladimir Oltean 2019-09-03 21:26 ` Vinicius Costa Gomes 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2019-09-03 10:12 UTC (permalink / raw) To: Eric Dumazet Cc: Gustavo A. R. Silva, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, netdev, lkml On Tue, 3 Sep 2019 at 10:19, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 9/3/19 3:08 AM, Gustavo A. R. Silva wrote: > > Add suffix LL to constant 1000 in order to avoid a potential integer > > overflow and give the compiler complete information about the proper > > arithmetic to use. Notice that this constant is being used in a context > > that expects an expression of type s64, but it's currently evaluated > > using 32-bit arithmetic. > > > > Addresses-Coverity-ID: 1453459 ("Unintentional integer overflow") > > Fixes: f04b514c0ce2 ("taprio: Set default link speed to 10 Mbps in taprio_set_picos_per_byte") > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > > --- > > net/sched/sch_taprio.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c > > index 8d8bc2ec5cd6..956f837436ea 100644 > > --- a/net/sched/sch_taprio.c > > +++ b/net/sched/sch_taprio.c > > @@ -966,7 +966,7 @@ static void taprio_set_picos_per_byte(struct net_device *dev, > > > > skip: > > picos_per_byte = div64_s64(NSEC_PER_SEC * 1000LL * 8, > > - speed * 1000 * 1000); > > + speed * 1000LL * 1000); > > > > atomic64_set(&q->picos_per_byte, picos_per_byte); > > netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %lld, linkspeed: %d\n", > > > > But, why even multiplying by 1,000,000 in the first place, this seems silly, > a standard 32 bit divide could be used instead. > > -> > > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c > index 8d8bc2ec5cd6281d811fd5d8a5c5211ebb0edd73..944b1af3215668e927d486b6c6c65c4599fb9539 100644 > --- a/net/sched/sch_taprio.c > +++ b/net/sched/sch_taprio.c > @@ -965,8 +965,7 @@ static void taprio_set_picos_per_byte(struct net_device *dev, > speed = ecmd.base.speed; > > skip: > - picos_per_byte = div64_s64(NSEC_PER_SEC * 1000LL * 8, > - speed * 1000 * 1000); > + picos_per_byte = (USEC_PER_SEC * 8) / speed; > > atomic64_set(&q->picos_per_byte, picos_per_byte); > netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %lld, linkspeed: %d\n", > > > Right. And while we're at it, there's still the potential division-by-zero problem which I still don't know how to solve without implementing a full-blown __ethtool_get_link_ksettings parser that checks against all the possible outputs it can have under the "no carrier" condition - see "[RFC PATCH 1/1] phylink: Set speed to SPEED_UNKNOWN when there is no PHY connected" for details. And there's also a third fix to be made: the netdev_dbg should be made to print "speed" instead of "ecmd.base.speed". Thanks, -Vladimir ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte 2019-09-03 10:12 ` Vladimir Oltean @ 2019-09-03 21:26 ` Vinicius Costa Gomes 2019-09-05 20:47 ` Vladimir Oltean 0 siblings, 1 reply; 7+ messages in thread From: Vinicius Costa Gomes @ 2019-09-03 21:26 UTC (permalink / raw) To: Vladimir Oltean, Eric Dumazet Cc: Gustavo A. R. Silva, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, netdev, lkml Hi, Vladimir Oltean <olteanv@gmail.com> writes: > Right. And while we're at it, there's still the potential > division-by-zero problem which I still don't know how to solve without > implementing a full-blown __ethtool_get_link_ksettings parser that > checks against all the possible outputs it can have under the "no > carrier" condition - see "[RFC PATCH 1/1] phylink: Set speed to > SPEED_UNKNOWN when there is no PHY connected" for details. > And there's also a third fix to be made: the netdev_dbg should be made > to print "speed" instead of "ecmd.base.speed". For the ksettings part I am thinking on adding something like this to ethtool.c. Do you think anything is missing (apart from the documentation)? -> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 95991e43..d37c80b 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -177,6 +177,9 @@ void ethtool_convert_legacy_u32_to_link_mode(unsigned long *dst, bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32, const unsigned long *src); +u32 ethtool_link_ksettings_to_speed(const struct ethtool_link_ksettings *settings, + u32 default_speed); + /** * struct ethtool_ops - optional netdev operations * @get_drvinfo: Report driver/device information. Should only set the diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 6288e69..80e3db3 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -539,6 +539,18 @@ struct ethtool_link_usettings { } link_modes; }; +u32 ethtool_link_ksettings_to_speed(const struct ethtool_link_ksettings *settings, + u32 default_speed) +{ + if (settings->base.speed == SPEED_UNKNOWN) + return default_speed; + + if (settings->base.speed == 0) + return default_speed; + + return settings->base.speed; +} + /* Internal kernel helper to query a device ethtool_link_settings. */ int __ethtool_get_link_ksettings(struct net_device *dev, struct ethtool_link_ksettings *link_ksettings) ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte 2019-09-03 21:26 ` Vinicius Costa Gomes @ 2019-09-05 20:47 ` Vladimir Oltean 2019-09-05 21:03 ` Gomes, Vinicius 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2019-09-05 20:47 UTC (permalink / raw) To: Vinicius Costa Gomes Cc: Eric Dumazet, Gustavo A. R. Silva, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, netdev, lkml Hi Vinicius, On Wed, 4 Sep 2019 at 00:26, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote: > > Hi, > > Vladimir Oltean <olteanv@gmail.com> writes: > > > Right. And while we're at it, there's still the potential > > division-by-zero problem which I still don't know how to solve without > > implementing a full-blown __ethtool_get_link_ksettings parser that > > checks against all the possible outputs it can have under the "no > > carrier" condition - see "[RFC PATCH 1/1] phylink: Set speed to > > SPEED_UNKNOWN when there is no PHY connected" for details. > > And there's also a third fix to be made: the netdev_dbg should be made > > to print "speed" instead of "ecmd.base.speed". > > For the ksettings part I am thinking on adding something like this to > ethtool.c. Do you think anything is missing (apart from the > documentation)? > > -> > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 95991e43..d37c80b 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -177,6 +177,9 @@ void ethtool_convert_legacy_u32_to_link_mode(unsigned long *dst, > bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32, > const unsigned long *src); > > +u32 ethtool_link_ksettings_to_speed(const struct ethtool_link_ksettings *settings, > + u32 default_speed); > + > /** > * struct ethtool_ops - optional netdev operations > * @get_drvinfo: Report driver/device information. Should only set the > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 6288e69..80e3db3 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -539,6 +539,18 @@ struct ethtool_link_usettings { > } link_modes; > }; > > +u32 ethtool_link_ksettings_to_speed(const struct ethtool_link_ksettings *settings, > + u32 default_speed) > +{ > + if (settings->base.speed == SPEED_UNKNOWN) > + return default_speed; > + > + if (settings->base.speed == 0) > + return default_speed; > + > + return settings->base.speed; > +} > + > /* Internal kernel helper to query a device ethtool_link_settings. */ > int __ethtool_get_link_ksettings(struct net_device *dev, > struct ethtool_link_ksettings *link_ksettings) Looks ok to me, but I have no saying over ethtool API. Actually I don't even know whom to ask - the output of ./scripts/get_maintainer.pl net/core/ethtool.c is a bit overwhelming. To avoid conflicts, there needs to be somebody out of us who takes Eric's simplification, with Gustavo's Reported-by tag, and the 2 ethtool & taprio patches to avoid division by zero, and the printing fix, and maybe do the same in cbs. Will you be the one? Should I? Thanks, -Vladimir ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte 2019-09-05 20:47 ` Vladimir Oltean @ 2019-09-05 21:03 ` Gomes, Vinicius 0 siblings, 0 replies; 7+ messages in thread From: Gomes, Vinicius @ 2019-09-05 21:03 UTC (permalink / raw) To: Vladimir Oltean Cc: Eric Dumazet, Gustavo A. R. Silva, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, netdev, lkml Hi Vladimir, > Looks ok to me, but I have no saying over ethtool API. Actually I don't even > know whom to ask - the output of ./scripts/get_maintainer.pl > net/core/ethtool.c is a bit overwhelming. > To avoid conflicts, there needs to be somebody out of us who takes Eric's > simplification, with Gustavo's Reported-by tag, and the 2 ethtool & taprio > patches to avoid division by zero, and the printing fix, and maybe do the same in > cbs. Will you be the one? Should I? If you have the cycles to do it, go for it. I would only be able to work on this next week. > > Thanks, > -Vladimir Thanks a lot, -- Vinicius ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-05 21:03 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-03 1:08 [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte Gustavo A. R. Silva 2019-09-03 1:20 ` Vladimir Oltean 2019-09-03 7:19 ` Eric Dumazet 2019-09-03 10:12 ` Vladimir Oltean 2019-09-03 21:26 ` Vinicius Costa Gomes 2019-09-05 20:47 ` Vladimir Oltean 2019-09-05 21:03 ` Gomes, Vinicius
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).