[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAM_iQpUa3a4gRLSwWpC-t98iv8bCu60cwHkx9AB=81g_9d_PWQ@mail.gmail.com>
Date: Thu, 14 Jan 2021 14:50:04 -0800
From: Cong Wang <xiyou.wangcong@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Linux Kernel Network Developers <netdev@...r.kernel.org>,
Cong Wang <cong.wang@...edance.com>,
syzbot <syzbot+2624e3778b18fc497c92@...kaller.appspotmail.com>,
Jamal Hadi Salim <jhs@...atatu.com>,
Xin Long <lucien.xin@...il.com>, Jiri Pirko <jiri@...nulli.us>
Subject: Re: [Patch net v3] cls_flower: call nla_ok() before nla_next()
On Thu, Jan 14, 2021 at 2:30 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Thu, 14 Jan 2021 13:57:13 -0800 Cong Wang wrote:
> > On Thu, Jan 14, 2021 at 1:36 PM Jakub Kicinski <kuba@...nel.org> wrote:
> > >
> > > On Thu, 14 Jan 2021 13:07:49 -0800 Cong Wang wrote:
> > > > - if (msk_depth)
> > > > - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth);
> > > > break;
> > > > default:
> > > > NL_SET_ERR_MSG(extack, "Unknown tunnel option type");
> > > > return -EINVAL;
> > > > }
> > > > +
> > > > + if (!nla_opt_msk)
> > > > + continue;
> > >
> > > Why the switch from !msk_depth to !nla_opt_msk?
> >
> > It is the same, when nla_opt_msk is NULL, msk_depth is 0.
> > Checking nla_opt_msk is NULL is more readable to express that
> > mask is not provided.
> >
> > >
> > > Seems like previously providing masks for only subset of options
> > > would have worked.
> >
> > I don't think so, every type has this check:
> >
> > if (key->enc_opts.len != mask->enc_opts.len) {
> > NL_SET_ERR_MSG(extack, "Key and mask
> > miss aligned");
> > return -EINVAL;
> > }
> >
> > which guarantees the numbers are aligned.
> >
> > Thanks.
>
> static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key,
> int depth, int option_len,
> struct netlink_ext_ack *extack)
> {
> struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_VXLAN_MAX + 1];
> struct vxlan_metadata *md;
> int err;
>
> md = (struct vxlan_metadata *)&key->enc_opts.data[key->enc_opts.len];
> memset(md, 0xff, sizeof(*md));
>
> if (!depth)
> return sizeof(*md);
> ^^^^^^^^^^^^^^^^^^^
>
> The mask is filled with all 1s if attribute is not provided.
Hmm, then what is the length comparison check for?
fl_set_vxlan_opt() either turns negative or sizeof(*md), and negitve
is already checked when it returns, so when we hit the length comparison
it is always equal. So it must be redundant.
(Note, I am only talking about the vxlan case you pick here, because the
geneve case is different, as it returns different sizes.)
Thanks.
Powered by blists - more mailing lists