| Message ID | 20230707220000.461410-2-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: fixes for sch_qfq | expand |
On Sat, Jul 8, 2023 at 12:01 AM Pedro Tammela <pctammela@mojatatu.com> wrote: > > 25369891fcef deletes a check for the case where no 'lmax' is > specified which 3037933448f6 previously fixed as 'lmax' > could be set to the device's MTU without any bound checking > for QFQ_LMAX_MIN and QFQ_LMAX_MAX. Therefore, reintroduce the check. > > Fixes: 25369891fcef ("net/sched: sch_qfq: refactor parsing of netlink parameters") > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > --- > net/sched/sch_qfq.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c > index dfd9a99e6257..63a5b277c117 100644 > --- a/net/sched/sch_qfq.c > +++ b/net/sched/sch_qfq.c > @@ -423,10 +423,17 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, > else > weight = 1; > > - if (tb[TCA_QFQ_LMAX]) > + if (tb[TCA_QFQ_LMAX]) { > lmax = nla_get_u32(tb[TCA_QFQ_LMAX]); > - else > + } else { > + /* MTU size is user controlled */ > lmax = psched_mtu(qdisc_dev(sch)); > + if (lmax < QFQ_MIN_LMAX || lmax > QFQ_MAX_LMAX) { > + NL_SET_ERR_MSG_MOD(extack, > + "MTU size out of bounds for qfq"); > + return -EINVAL; > + } > + } > Reviewed-by: Eric Dumazet <edumazet@google.com> Speaking of psched_mtu(), I see that net/sched/sch_pie.c is using it without holding RTNL, so dev->mtu can be changed underneath. KCSAN could issue a warning. Feel free to submit this fix (I am currently traveling) diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index e98aac9d5ad5737592ab7cd409c174707cd68681..15960564e0c364ef430f1e3fcdd0e835c2f94a77 100644 --- a/include/net/pkt_sched.h +++ b/include/net/pkt_sched.h @@ -134,7 +134,7 @@ extern const struct nla_policy rtm_tca_policy[TCA_MAX + 1]; */ static inline unsigned int psched_mtu(const struct net_device *dev) { - return dev->mtu + dev->hard_header_len; + return READ_ONCE(dev->mtu) + dev->hard_header_len; } static inline struct net *qdisc_net(struct Qdisc *q)
On 08/07/2023 05:14, Eric Dumazet wrote: > On Sat, Jul 8, 2023 at 12:01 AM Pedro Tammela <pctammela@mojatatu.com> wrote: >> >> 25369891fcef deletes a check for the case where no 'lmax' is >> specified which 3037933448f6 previously fixed as 'lmax' >> could be set to the device's MTU without any bound checking >> for QFQ_LMAX_MIN and QFQ_LMAX_MAX. Therefore, reintroduce the check. >> >> Fixes: 25369891fcef ("net/sched: sch_qfq: refactor parsing of netlink parameters") >> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> >> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> >> --- >> net/sched/sch_qfq.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c >> index dfd9a99e6257..63a5b277c117 100644 >> --- a/net/sched/sch_qfq.c >> +++ b/net/sched/sch_qfq.c >> @@ -423,10 +423,17 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, >> else >> weight = 1; >> >> - if (tb[TCA_QFQ_LMAX]) >> + if (tb[TCA_QFQ_LMAX]) { >> lmax = nla_get_u32(tb[TCA_QFQ_LMAX]); >> - else >> + } else { >> + /* MTU size is user controlled */ >> lmax = psched_mtu(qdisc_dev(sch)); >> + if (lmax < QFQ_MIN_LMAX || lmax > QFQ_MAX_LMAX) { >> + NL_SET_ERR_MSG_MOD(extack, >> + "MTU size out of bounds for qfq"); >> + return -EINVAL; >> + } >> + } >> > > Reviewed-by: Eric Dumazet <edumazet@google.com> > > Speaking of psched_mtu(), I see that net/sched/sch_pie.c is using it > without holding RTNL, > so dev->mtu can be changed underneath. KCSAN could issue a warning. > > Feel free to submit this fix (I am currently traveling) Sure! Thank you Eric. > > diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h > index e98aac9d5ad5737592ab7cd409c174707cd68681..15960564e0c364ef430f1e3fcdd0e835c2f94a77 > 100644 > --- a/include/net/pkt_sched.h > +++ b/include/net/pkt_sched.h > @@ -134,7 +134,7 @@ extern const struct nla_policy rtm_tca_policy[TCA_MAX + 1]; > */ > static inline unsigned int psched_mtu(const struct net_device *dev) > { > - return dev->mtu + dev->hard_header_len; > + return READ_ONCE(dev->mtu) + dev->hard_header_len; > } > > static inline struct net *qdisc_net(struct Qdisc *q)
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c index dfd9a99e6257..63a5b277c117 100644 --- a/net/sched/sch_qfq.c +++ b/net/sched/sch_qfq.c @@ -423,10 +423,17 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, else weight = 1; - if (tb[TCA_QFQ_LMAX]) + if (tb[TCA_QFQ_LMAX]) { lmax = nla_get_u32(tb[TCA_QFQ_LMAX]); - else + } else { + /* MTU size is user controlled */ lmax = psched_mtu(qdisc_dev(sch)); + if (lmax < QFQ_MIN_LMAX || lmax > QFQ_MAX_LMAX) { + NL_SET_ERR_MSG_MOD(extack, + "MTU size out of bounds for qfq"); + return -EINVAL; + } + } inv_w = ONE_FP / weight; weight = ONE_FP / inv_w;
25369891fcef deletes a check for the case where no 'lmax' is specified which 3037933448f6 previously fixed as 'lmax' could be set to the device's MTU without any bound checking for QFQ_LMAX_MIN and QFQ_LMAX_MAX. Therefore, reintroduce the check. Fixes: 25369891fcef ("net/sched: sch_qfq: refactor parsing of netlink parameters") Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> --- net/sched/sch_qfq.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)