Thu, Oct 10, 2019 at 07:21:02PM CEST, jakub.kicin...@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 <mkube...@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; >> }