* [PATCH net-next] genetlink: do not parse attributes for families with zero maxattr
@ 2019-10-09 16:44 Michal Kubecek
2019-10-10 8:39 ` Jiri Pirko
2019-10-10 9:31 ` Jiri Pirko
0 siblings, 2 replies; 6+ messages in thread
From: Michal Kubecek @ 2019-10-09 16:44 UTC (permalink / raw)
To: David S. Miller; +Cc: Jiri Pirko, Johannes Berg, netdev, linux-kernel
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 | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index ecc2bd3e73e4..c4bf8830eedf 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;
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next] genetlink: do not parse attributes for families with zero maxattr 2019-10-09 16:44 [PATCH net-next] genetlink: do not parse attributes for families with zero maxattr Michal Kubecek @ 2019-10-10 8:39 ` Jiri Pirko 2019-10-10 9:13 ` Michal Kubecek 2019-10-10 9:31 ` Jiri Pirko 1 sibling, 1 reply; 6+ messages in thread From: Jiri Pirko @ 2019-10-10 8:39 UTC (permalink / raw) To: Michal Kubecek Cc: David S. Miller, Jiri Pirko, Johannes Berg, netdev, linux-kernel Wed, Oct 09, 2019 at 06:44:32PM 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(). This is the original code before the changes: if (ops->doit == NULL) return -EOPNOTSUPP; if (family->maxattr && family->parallel_ops) { attrbuf = kmalloc_array(family->maxattr + 1, sizeof(struct nlattr *), GFP_KERNEL); if (attrbuf == NULL) return -ENOMEM; } else attrbuf = family->attrbuf; if (attrbuf) { enum netlink_validation validate = NL_VALIDATE_STRICT; if (ops->validate & GENL_DONT_VALIDATE_STRICT) validate = NL_VALIDATE_LIBERAL; err = __nlmsg_parse(nlh, hdrlen, attrbuf, family->maxattr, family->policy, validate, extack); if (err < 0) goto out; } Looks like the __nlmsg_parse() is called no matter if maxattr if 0 or not. It is only considered for allocation of attrbuf. This is in-sync with the current code. For dumpit, the check was there: if (family->maxattr) { unsigned int validate = NL_VALIDATE_STRICT; if (ops->validate & GENL_DONT_VALIDATE_DUMP_STRICT) validate = NL_VALIDATE_LIBERAL; rc = __nla_validate(nlmsg_attrdata(nlh, hdrlen), nlmsg_attrlen(nlh, hdrlen), family->maxattr, family->policy, validate, extack); if (rc) return rc; } > >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 | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > >diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c >index ecc2bd3e73e4..c4bf8830eedf 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; >-- >2.23.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] genetlink: do not parse attributes for families with zero maxattr 2019-10-10 8:39 ` Jiri Pirko @ 2019-10-10 9:13 ` Michal Kubecek 2019-10-10 9:30 ` Jiri Pirko 0 siblings, 1 reply; 6+ messages in thread From: Michal Kubecek @ 2019-10-10 9:13 UTC (permalink / raw) To: Jiri Pirko Cc: David S. Miller, Jiri Pirko, Johannes Berg, netdev, linux-kernel On Thu, Oct 10, 2019 at 10:39:58AM +0200, Jiri Pirko wrote: > Wed, Oct 09, 2019 at 06:44:32PM 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(). > > This is the original code before the changes: > > if (ops->doit == NULL) > return -EOPNOTSUPP; > > if (family->maxattr && family->parallel_ops) { > attrbuf = kmalloc_array(family->maxattr + 1, > sizeof(struct nlattr *), > GFP_KERNEL); > if (attrbuf == NULL) > return -ENOMEM; > } else > attrbuf = family->attrbuf; > > if (attrbuf) { > enum netlink_validation validate = NL_VALIDATE_STRICT; > > if (ops->validate & GENL_DONT_VALIDATE_STRICT) > validate = NL_VALIDATE_LIBERAL; > > err = __nlmsg_parse(nlh, hdrlen, attrbuf, family->maxattr, > family->policy, validate, extack); > if (err < 0) > goto out; > } > > Looks like the __nlmsg_parse() is called no matter if maxattr if 0 or > not. It is only considered for allocation of attrbuf. This is in-sync > with the current code. If family->maxattr is 0, genl_register_family() sets family->attrbuf to NULL so that attrbuf is also NULL and the whole "if (attrbuf) { ... }" block is not executed. Michal ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] genetlink: do not parse attributes for families with zero maxattr 2019-10-10 9:13 ` Michal Kubecek @ 2019-10-10 9:30 ` Jiri Pirko 0 siblings, 0 replies; 6+ messages in thread From: Jiri Pirko @ 2019-10-10 9:30 UTC (permalink / raw) To: Michal Kubecek Cc: David S. Miller, Jiri Pirko, Johannes Berg, netdev, linux-kernel Thu, Oct 10, 2019 at 11:13:47AM CEST, mkubecek@suse.cz wrote: >On Thu, Oct 10, 2019 at 10:39:58AM +0200, Jiri Pirko wrote: >> Wed, Oct 09, 2019 at 06:44:32PM 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(). >> >> This is the original code before the changes: >> >> if (ops->doit == NULL) >> return -EOPNOTSUPP; >> >> if (family->maxattr && family->parallel_ops) { >> attrbuf = kmalloc_array(family->maxattr + 1, >> sizeof(struct nlattr *), >> GFP_KERNEL); >> if (attrbuf == NULL) >> return -ENOMEM; >> } else >> attrbuf = family->attrbuf; >> >> if (attrbuf) { >> enum netlink_validation validate = NL_VALIDATE_STRICT; >> >> if (ops->validate & GENL_DONT_VALIDATE_STRICT) >> validate = NL_VALIDATE_LIBERAL; >> >> err = __nlmsg_parse(nlh, hdrlen, attrbuf, family->maxattr, >> family->policy, validate, extack); >> if (err < 0) >> goto out; >> } >> >> Looks like the __nlmsg_parse() is called no matter if maxattr if 0 or >> not. It is only considered for allocation of attrbuf. This is in-sync >> with the current code. > >If family->maxattr is 0, genl_register_family() sets family->attrbuf to >NULL so that attrbuf is also NULL and the whole "if (attrbuf) { ... }" >block is not executed. Ah, I missed that. Thanks! > >Michal ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] genetlink: do not parse attributes for families with zero maxattr 2019-10-09 16:44 [PATCH net-next] genetlink: do not parse attributes for families with zero maxattr Michal Kubecek 2019-10-10 8:39 ` Jiri Pirko @ 2019-10-10 9:31 ` Jiri Pirko 2019-10-10 10:45 ` Michal Kubecek 1 sibling, 1 reply; 6+ messages in thread From: Jiri Pirko @ 2019-10-10 9:31 UTC (permalink / raw) To: Michal Kubecek Cc: David S. Miller, Jiri Pirko, Johannes Berg, netdev, linux-kernel Wed, Oct 09, 2019 at 06:44:32PM 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> >--- > net/netlink/genetlink.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > >diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c >index ecc2bd3e73e4..c4bf8830eedf 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); Please also adjust genl_family_rcv_msg_attrs_free() call arg below in this function in the similar way. > 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; >-- >2.23.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] genetlink: do not parse attributes for families with zero maxattr 2019-10-10 9:31 ` Jiri Pirko @ 2019-10-10 10:45 ` Michal Kubecek 0 siblings, 0 replies; 6+ messages in thread From: Michal Kubecek @ 2019-10-10 10:45 UTC (permalink / raw) To: netdev Cc: Jiri Pirko, David S. Miller, Jiri Pirko, Johannes Berg, linux-kernel On Thu, Oct 10, 2019 at 11:31:53AM +0200, Jiri Pirko wrote: > Wed, Oct 09, 2019 at 06:44:32PM 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> > >--- > > net/netlink/genetlink.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > >diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c > >index ecc2bd3e73e4..c4bf8830eedf 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); > > Please also adjust genl_family_rcv_msg_attrs_free() call arg > below in this function in the similar way. Sent v2. Michal ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-10 10:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-09 16:44 [PATCH net-next] genetlink: do not parse attributes for families with zero maxattr Michal Kubecek 2019-10-10 8:39 ` Jiri Pirko 2019-10-10 9:13 ` Michal Kubecek 2019-10-10 9:30 ` Jiri Pirko 2019-10-10 9:31 ` Jiri Pirko 2019-10-10 10:45 ` Michal Kubecek
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).