| Message ID | 20191010103402.36408E378C@unicorn.suse.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v2] genetlink: do not parse attributes for families with zero maxattr | expand |
Thu, Oct 10, 2019 at 12:34:02PM CEST, mkubecek@suse.cz wrote: >Commit c10e6cf85e7d ("net: genetlink: push attrbuf allocation and parsing >to a separate function") moved attribute buffer allocation and attribute >parsing from genl_family_rcv_msg_doit() into a separate function >genl_family_rcv_msg_attrs_parse() which, unlike the previous code, calls >__nlmsg_parse() even if family->maxattr is 0 (i.e. the family does its own >parsing). The parser error is ignored and does not propagate out of >genl_family_rcv_msg_attrs_parse() but an error message ("Unknown attribute >type") is set in extack and if further processing generates no error or >warning, it stays there and is interpreted as a warning by userspace. > >Dumpit requests are not affected as genl_family_rcv_msg_dumpit() bypasses >the call of genl_family_rcv_msg_doit() if family->maxattr is zero. Do the >same also in genl_family_rcv_msg_doit(). > >Fixes: c10e6cf85e7d ("net: genetlink: push attrbuf allocation and parsing to a separate function") >Signed-off-by: Michal Kubecek <mkubecek@suse.cz> Acked-by: Jiri Pirko <jiri@mellanox.com> Thanks!
On Thu, 10 Oct 2019 12:34:02 +0200 (CEST), Michal Kubecek wrote: > Commit c10e6cf85e7d ("net: genetlink: push attrbuf allocation and parsing > to a separate function") moved attribute buffer allocation and attribute > parsing from genl_family_rcv_msg_doit() into a separate function > genl_family_rcv_msg_attrs_parse() which, unlike the previous code, calls > __nlmsg_parse() even if family->maxattr is 0 (i.e. the family does its own > parsing). The parser error is ignored and does not propagate out of > genl_family_rcv_msg_attrs_parse() but an error message ("Unknown attribute > type") is set in extack and if further processing generates no error or > warning, it stays there and is interpreted as a warning by userspace. > > Dumpit requests are not affected as genl_family_rcv_msg_dumpit() bypasses > the call of genl_family_rcv_msg_doit() if family->maxattr is zero. Do the > same also in genl_family_rcv_msg_doit(). > > Fixes: c10e6cf85e7d ("net: genetlink: push attrbuf allocation and parsing to a separate function") > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> > --- > net/netlink/genetlink.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c > index ecc2bd3e73e4..1f14e55ad3ad 100644 > --- a/net/netlink/genetlink.c > +++ b/net/netlink/genetlink.c > @@ -639,21 +639,23 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family, > const struct genl_ops *ops, > int hdrlen, struct net *net) > { > - struct nlattr **attrbuf; > + struct nlattr **attrbuf = NULL; > struct genl_info info; > int err; > > if (!ops->doit) > return -EOPNOTSUPP; > > + if (!family->maxattr) > + goto no_attrs; > attrbuf = genl_family_rcv_msg_attrs_parse(family, nlh, extack, > ops, hdrlen, > GENL_DONT_VALIDATE_STRICT, > - family->maxattr && > family->parallel_ops); > if (IS_ERR(attrbuf)) > return PTR_ERR(attrbuf); > > +no_attrs: The use of a goto statement as a replacement for an if is making me uncomfortable. Looks like both callers of genl_family_rcv_msg_attrs_parse() jump around it if !family->maxattr and then check the result with IS_ERR(). Would it not make more sense to have genl_family_rcv_msg_attrs_parse() return NULL if !family->maxattr? Just wondering, if you guys prefer this version I can apply.. > info.snd_seq = nlh->nlmsg_seq; > info.snd_portid = NETLINK_CB(skb).portid; > info.nlhdr = nlh; > @@ -676,8 +678,7 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family, > family->post_doit(ops, skb, &info); > > out: > - genl_family_rcv_msg_attrs_free(family, attrbuf, > - family->maxattr && family->parallel_ops); > + genl_family_rcv_msg_attrs_free(family, attrbuf, family->parallel_ops); > > return err; > }
On Thu, Oct 10, 2019 at 10:21:02AM -0700, Jakub Kicinski wrote: > On Thu, 10 Oct 2019 12:34:02 +0200 (CEST), Michal Kubecek wrote: > > Commit c10e6cf85e7d ("net: genetlink: push attrbuf allocation and parsing > > to a separate function") moved attribute buffer allocation and attribute > > parsing from genl_family_rcv_msg_doit() into a separate function > > genl_family_rcv_msg_attrs_parse() which, unlike the previous code, calls > > __nlmsg_parse() even if family->maxattr is 0 (i.e. the family does its own > > parsing). The parser error is ignored and does not propagate out of > > genl_family_rcv_msg_attrs_parse() but an error message ("Unknown attribute > > type") is set in extack and if further processing generates no error or > > warning, it stays there and is interpreted as a warning by userspace. > > > > Dumpit requests are not affected as genl_family_rcv_msg_dumpit() bypasses > > the call of genl_family_rcv_msg_doit() if family->maxattr is zero. Do the > > same also in genl_family_rcv_msg_doit(). > > > > Fixes: c10e6cf85e7d ("net: genetlink: push attrbuf allocation and parsing to a separate function") > > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> > > --- > > net/netlink/genetlink.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c > > index ecc2bd3e73e4..1f14e55ad3ad 100644 > > --- a/net/netlink/genetlink.c > > +++ b/net/netlink/genetlink.c > > @@ -639,21 +639,23 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family, > > const struct genl_ops *ops, > > int hdrlen, struct net *net) > > { > > - struct nlattr **attrbuf; > > + struct nlattr **attrbuf = NULL; > > struct genl_info info; > > int err; > > > > if (!ops->doit) > > return -EOPNOTSUPP; > > > > + if (!family->maxattr) > > + goto no_attrs; > > attrbuf = genl_family_rcv_msg_attrs_parse(family, nlh, extack, > > ops, hdrlen, > > GENL_DONT_VALIDATE_STRICT, > > - family->maxattr && > > family->parallel_ops); > > if (IS_ERR(attrbuf)) > > return PTR_ERR(attrbuf); > > > > +no_attrs: > > The use of a goto statement as a replacement for an if is making me > uncomfortable. I used instead of a simple if because (1) it's what the dumpit code does and (2) the function call arguments are already quite pressed to the 80-character barrier. > Looks like both callers of genl_family_rcv_msg_attrs_parse() jump > around it if !family->maxattr and then check the result with IS_ERR(). > > Would it not make more sense to have genl_family_rcv_msg_attrs_parse() > return NULL if !family->maxattr? This sounds like a good solution. I'll check again in the morning and send v3. Michal
Thu, Oct 10, 2019 at 07:21:02PM CEST, jakub.kicinski@netronome.com wrote: >On Thu, 10 Oct 2019 12:34:02 +0200 (CEST), Michal Kubecek wrote: >> Commit c10e6cf85e7d ("net: genetlink: push attrbuf allocation and parsing >> to a separate function") moved attribute buffer allocation and attribute >> parsing from genl_family_rcv_msg_doit() into a separate function >> genl_family_rcv_msg_attrs_parse() which, unlike the previous code, calls >> __nlmsg_parse() even if family->maxattr is 0 (i.e. the family does its own >> parsing). The parser error is ignored and does not propagate out of >> genl_family_rcv_msg_attrs_parse() but an error message ("Unknown attribute >> type") is set in extack and if further processing generates no error or >> warning, it stays there and is interpreted as a warning by userspace. >> >> Dumpit requests are not affected as genl_family_rcv_msg_dumpit() bypasses >> the call of genl_family_rcv_msg_doit() if family->maxattr is zero. Do the >> same also in genl_family_rcv_msg_doit(). >> >> Fixes: c10e6cf85e7d ("net: genetlink: push attrbuf allocation and parsing to a separate function") >> Signed-off-by: Michal Kubecek <mkubecek@suse.cz> >> --- >> net/netlink/genetlink.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c >> index ecc2bd3e73e4..1f14e55ad3ad 100644 >> --- a/net/netlink/genetlink.c >> +++ b/net/netlink/genetlink.c >> @@ -639,21 +639,23 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family, >> const struct genl_ops *ops, >> int hdrlen, struct net *net) >> { >> - struct nlattr **attrbuf; >> + struct nlattr **attrbuf = NULL; >> struct genl_info info; >> int err; >> >> if (!ops->doit) >> return -EOPNOTSUPP; >> >> + if (!family->maxattr) >> + goto no_attrs; >> attrbuf = genl_family_rcv_msg_attrs_parse(family, nlh, extack, >> ops, hdrlen, >> GENL_DONT_VALIDATE_STRICT, >> - family->maxattr && >> family->parallel_ops); >> if (IS_ERR(attrbuf)) >> return PTR_ERR(attrbuf); >> >> +no_attrs: > >The use of a goto statement as a replacement for an if is making me >uncomfortable. > >Looks like both callers of genl_family_rcv_msg_attrs_parse() jump >around it if !family->maxattr and then check the result with IS_ERR(). > >Would it not make more sense to have genl_family_rcv_msg_attrs_parse() >return NULL if !family->maxattr? Okay. Sounds fine to me. > >Just wondering, if you guys prefer this version I can apply.. > >> info.snd_seq = nlh->nlmsg_seq; >> info.snd_portid = NETLINK_CB(skb).portid; >> info.nlhdr = nlh; >> @@ -676,8 +678,7 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family, >> family->post_doit(ops, skb, &info); >> >> out: >> - genl_family_rcv_msg_attrs_free(family, attrbuf, >> - family->maxattr && family->parallel_ops); >> + genl_family_rcv_msg_attrs_free(family, attrbuf, family->parallel_ops); >> >> return err; >> }
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index ecc2bd3e73e4..1f14e55ad3ad 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -639,21 +639,23 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family, const struct genl_ops *ops, int hdrlen, struct net *net) { - struct nlattr **attrbuf; + struct nlattr **attrbuf = NULL; struct genl_info info; int err; if (!ops->doit) return -EOPNOTSUPP; + if (!family->maxattr) + goto no_attrs; attrbuf = genl_family_rcv_msg_attrs_parse(family, nlh, extack, ops, hdrlen, GENL_DONT_VALIDATE_STRICT, - family->maxattr && family->parallel_ops); if (IS_ERR(attrbuf)) return PTR_ERR(attrbuf); +no_attrs: info.snd_seq = nlh->nlmsg_seq; info.snd_portid = NETLINK_CB(skb).portid; info.nlhdr = nlh; @@ -676,8 +678,7 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family, family->post_doit(ops, skb, &info); out: - genl_family_rcv_msg_attrs_free(family, attrbuf, - family->maxattr && family->parallel_ops); + genl_family_rcv_msg_attrs_free(family, attrbuf, family->parallel_ops); return err; }
Commit c10e6cf85e7d ("net: genetlink: push attrbuf allocation and parsing to a separate function") moved attribute buffer allocation and attribute parsing from genl_family_rcv_msg_doit() into a separate function genl_family_rcv_msg_attrs_parse() which, unlike the previous code, calls __nlmsg_parse() even if family->maxattr is 0 (i.e. the family does its own parsing). The parser error is ignored and does not propagate out of genl_family_rcv_msg_attrs_parse() but an error message ("Unknown attribute type") is set in extack and if further processing generates no error or warning, it stays there and is interpreted as a warning by userspace. Dumpit requests are not affected as genl_family_rcv_msg_dumpit() bypasses the call of genl_family_rcv_msg_doit() if family->maxattr is zero. Do the same also in genl_family_rcv_msg_doit(). Fixes: c10e6cf85e7d ("net: genetlink: push attrbuf allocation and parsing to a separate function") Signed-off-by: Michal Kubecek <mkubecek@suse.cz> --- net/netlink/genetlink.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)