* [PATCH nf-next 0/3] netfilter: fix some small bugs related to nft_log
@ 2016-07-18 12:44 Liping Zhang
2016-07-18 12:44 ` [PATCH nf-next 1/3] netfilter: nft_log: fix possible memory leak if log expr init fail Liping Zhang
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Liping Zhang @ 2016-07-18 12:44 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, Liping Zhang
From: Liping Zhang <liping.zhang@spreadtrum.com>
This patchset is very small, aim to fix some bugs related to nftables log expr.
patch#1 fix a possible memory leak if the user specify the log prefix but the log
expr init fail.
patch#2 add a validity check of log level, otherwise user can specify an invalid
log level.
patch#3 fix "nft add rule filter input log snaplen 100" cannot work successfully,
the reason is similar to xt_NFLOG.
Liping Zhang (3):
netfilter: nft_log: fix possible memory leak if log expr init fail
netfilter: nft_log: check the validity of log level
netfilter: nft_log: fix snaplen does not truncate packets
net/netfilter/nft_log.c | 34 +++++++++++++++++++++++++---------
1 file changed, 25 insertions(+), 9 deletions(-)
--
2.5.5
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH nf-next 1/3] netfilter: nft_log: fix possible memory leak if log expr init fail
2016-07-18 12:44 [PATCH nf-next 0/3] netfilter: fix some small bugs related to nft_log Liping Zhang
@ 2016-07-18 12:44 ` Liping Zhang
2016-07-19 18:13 ` Pablo Neira Ayuso
2016-07-18 12:44 ` [PATCH nf-next 2/3] netfilter: nft_log: check the validity of log level Liping Zhang
2016-07-18 12:44 ` [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets Liping Zhang
2 siblings, 1 reply; 11+ messages in thread
From: Liping Zhang @ 2016-07-18 12:44 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, Liping Zhang
From: Liping Zhang <liping.zhang@spreadtrum.com>
Suppose that we specify the NFTA_LOG_PREFIX, then NFTA_LOG_LEVEL
and NFTA_LOG_GROUP are specified together or nf_logger_find_get
call returns fail, i.e. expr init fail, memory leak will happen.
Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
net/netfilter/nft_log.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
index 713d668..e1b34ff 100644
--- a/net/netfilter/nft_log.c
+++ b/net/netfilter/nft_log.c
@@ -52,6 +52,14 @@ static int nft_log_init(const struct nft_ctx *ctx,
struct nft_log *priv = nft_expr_priv(expr);
struct nf_loginfo *li = &priv->loginfo;
const struct nlattr *nla;
+ int err;
+
+ li->type = NF_LOG_TYPE_LOG;
+ if (tb[NFTA_LOG_LEVEL] != NULL &&
+ tb[NFTA_LOG_GROUP] != NULL)
+ return -EINVAL;
+ if (tb[NFTA_LOG_GROUP] != NULL)
+ li->type = NF_LOG_TYPE_ULOG;
nla = tb[NFTA_LOG_PREFIX];
if (nla != NULL) {
@@ -63,13 +71,6 @@ static int nft_log_init(const struct nft_ctx *ctx,
priv->prefix = (char *)nft_log_null_prefix;
}
- li->type = NF_LOG_TYPE_LOG;
- if (tb[NFTA_LOG_LEVEL] != NULL &&
- tb[NFTA_LOG_GROUP] != NULL)
- return -EINVAL;
- if (tb[NFTA_LOG_GROUP] != NULL)
- li->type = NF_LOG_TYPE_ULOG;
-
switch (li->type) {
case NF_LOG_TYPE_LOG:
if (tb[NFTA_LOG_LEVEL] != NULL) {
@@ -96,7 +97,16 @@ static int nft_log_init(const struct nft_ctx *ctx,
break;
}
- return nf_logger_find_get(ctx->afi->family, li->type);
+ err = nf_logger_find_get(ctx->afi->family, li->type);
+ if (err < 0)
+ goto err1;
+
+ return 0;
+
+err1:
+ if (priv->prefix != nft_log_null_prefix)
+ kfree(priv->prefix);
+ return err;
}
static void nft_log_destroy(const struct nft_ctx *ctx,
--
2.5.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH nf-next 2/3] netfilter: nft_log: check the validity of log level
2016-07-18 12:44 [PATCH nf-next 0/3] netfilter: fix some small bugs related to nft_log Liping Zhang
2016-07-18 12:44 ` [PATCH nf-next 1/3] netfilter: nft_log: fix possible memory leak if log expr init fail Liping Zhang
@ 2016-07-18 12:44 ` Liping Zhang
2016-07-19 18:14 ` Pablo Neira Ayuso
2016-07-18 12:44 ` [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets Liping Zhang
2 siblings, 1 reply; 11+ messages in thread
From: Liping Zhang @ 2016-07-18 12:44 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, Liping Zhang
From: Liping Zhang <liping.zhang@spreadtrum.com>
User can specify the log level larger than 7(debug level) via
nfnetlink, this is invalid. So in this case, we should report
EINVAL to the userspace.
Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
net/netfilter/nft_log.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
index e1b34ff..5f6f088 100644
--- a/net/netfilter/nft_log.c
+++ b/net/netfilter/nft_log.c
@@ -79,6 +79,11 @@ static int nft_log_init(const struct nft_ctx *ctx,
} else {
li->u.log.level = LOGLEVEL_WARNING;
}
+ if (li->u.log.level > LOGLEVEL_DEBUG) {
+ err = -EINVAL;
+ goto err1;
+ }
+
if (tb[NFTA_LOG_FLAGS] != NULL) {
li->u.log.logflags =
ntohl(nla_get_be32(tb[NFTA_LOG_FLAGS]));
--
2.5.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets
2016-07-18 12:44 [PATCH nf-next 0/3] netfilter: fix some small bugs related to nft_log Liping Zhang
2016-07-18 12:44 ` [PATCH nf-next 1/3] netfilter: nft_log: fix possible memory leak if log expr init fail Liping Zhang
2016-07-18 12:44 ` [PATCH nf-next 2/3] netfilter: nft_log: check the validity of log level Liping Zhang
@ 2016-07-18 12:44 ` Liping Zhang
2016-07-19 18:16 ` Pablo Neira Ayuso
2 siblings, 1 reply; 11+ messages in thread
From: Liping Zhang @ 2016-07-18 12:44 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, Liping Zhang
From: Liping Zhang <liping.zhang@spreadtrum.com>
There's a similar problem in xt_NFLOG, and was fixed by commit 7643507fe8b5
("netfilter: xt_NFLOG: nflog-range does not truncate packets"). Only set
copy_len here does not work, so we should enable NF_LOG_F_COPY_LEN also.
Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
net/netfilter/nft_log.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
index 5f6f088..24a73bb 100644
--- a/net/netfilter/nft_log.c
+++ b/net/netfilter/nft_log.c
@@ -92,6 +92,7 @@ static int nft_log_init(const struct nft_ctx *ctx,
case NF_LOG_TYPE_ULOG:
li->u.ulog.group = ntohs(nla_get_be16(tb[NFTA_LOG_GROUP]));
if (tb[NFTA_LOG_SNAPLEN] != NULL) {
+ li->u.ulog.flags |= NF_LOG_F_COPY_LEN;
li->u.ulog.copy_len =
ntohl(nla_get_be32(tb[NFTA_LOG_SNAPLEN]));
}
@@ -149,7 +150,7 @@ static int nft_log_dump(struct sk_buff *skb, const struct nft_expr *expr)
if (nla_put_be16(skb, NFTA_LOG_GROUP, htons(li->u.ulog.group)))
goto nla_put_failure;
- if (li->u.ulog.copy_len) {
+ if (li->u.ulog.flags & NF_LOG_F_COPY_LEN) {
if (nla_put_be32(skb, NFTA_LOG_SNAPLEN,
htonl(li->u.ulog.copy_len)))
goto nla_put_failure;
--
2.5.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH nf-next 1/3] netfilter: nft_log: fix possible memory leak if log expr init fail
2016-07-18 12:44 ` [PATCH nf-next 1/3] netfilter: nft_log: fix possible memory leak if log expr init fail Liping Zhang
@ 2016-07-19 18:13 ` Pablo Neira Ayuso
0 siblings, 0 replies; 11+ messages in thread
From: Pablo Neira Ayuso @ 2016-07-19 18:13 UTC (permalink / raw)
To: Liping Zhang; +Cc: netfilter-devel, Liping Zhang
On Mon, Jul 18, 2016 at 08:44:15PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
>
> Suppose that we specify the NFTA_LOG_PREFIX, then NFTA_LOG_LEVEL
> and NFTA_LOG_GROUP are specified together or nf_logger_find_get
> call returns fail, i.e. expr init fail, memory leak will happen.
Applied, thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nf-next 2/3] netfilter: nft_log: check the validity of log level
2016-07-18 12:44 ` [PATCH nf-next 2/3] netfilter: nft_log: check the validity of log level Liping Zhang
@ 2016-07-19 18:14 ` Pablo Neira Ayuso
0 siblings, 0 replies; 11+ messages in thread
From: Pablo Neira Ayuso @ 2016-07-19 18:14 UTC (permalink / raw)
To: Liping Zhang; +Cc: netfilter-devel, Liping Zhang
On Mon, Jul 18, 2016 at 08:44:16PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
>
> User can specify the log level larger than 7(debug level) via
> nfnetlink, this is invalid. So in this case, we should report
> EINVAL to the userspace.
Also applied, thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets
2016-07-18 12:44 ` [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets Liping Zhang
@ 2016-07-19 18:16 ` Pablo Neira Ayuso
2016-07-19 23:00 ` Liping Zhang
0 siblings, 1 reply; 11+ messages in thread
From: Pablo Neira Ayuso @ 2016-07-19 18:16 UTC (permalink / raw)
To: Liping Zhang; +Cc: netfilter-devel, Liping Zhang
On Mon, Jul 18, 2016 at 08:44:17PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
>
> There's a similar problem in xt_NFLOG, and was fixed by commit 7643507fe8b5
> ("netfilter: xt_NFLOG: nflog-range does not truncate packets"). Only set
> copy_len here does not work, so we should enable NF_LOG_F_COPY_LEN also.
Applied, thanks.
Will you send me a patch for nftables userspace to enable this flag?
It would be good to update the translation to make sure --nflog-size
map to snaplen and ignore --nflog-range.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets
2016-07-19 18:16 ` Pablo Neira Ayuso
@ 2016-07-19 23:00 ` Liping Zhang
2016-07-20 8:25 ` Pablo Neira Ayuso
0 siblings, 1 reply; 11+ messages in thread
From: Liping Zhang @ 2016-07-19 23:00 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, Liping Zhang
At 2016-07-20 02:16:00, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
>On Mon, Jul 18, 2016 at 08:44:17PM +0800, Liping Zhang wrote:
>> From: Liping Zhang <liping.zhang@spreadtrum.com>
>>
>> There's a similar problem in xt_NFLOG, and was fixed by commit 7643507fe8b5
>> ("netfilter: xt_NFLOG: nflog-range does not truncate packets"). Only set
>> copy_len here does not work, so we should enable NF_LOG_F_COPY_LEN also.
>
>Applied, thanks.
>
>Will you send me a patch for nftables userspace to enable this flag?
>
>It would be good to update the translation to make sure --nflog-size
>map to snaplen and ignore --nflog-range.
I find that nftables already support this feature, the following command mean to truncate packets
to 100 bytes before logging to the userspace:
#nft add rule filter input log group 0 snaplen 100
Before my patch, it does not work.
And after apply my patch, it works as expected.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets
2016-07-19 23:00 ` Liping Zhang
@ 2016-07-20 8:25 ` Pablo Neira Ayuso
2016-07-20 9:00 ` Liping Zhang
0 siblings, 1 reply; 11+ messages in thread
From: Pablo Neira Ayuso @ 2016-07-20 8:25 UTC (permalink / raw)
To: Liping Zhang; +Cc: netfilter-devel, Liping Zhang
On Wed, Jul 20, 2016 at 07:00:13AM +0800, Liping Zhang wrote:
> At 2016-07-20 02:16:00, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
> >On Mon, Jul 18, 2016 at 08:44:17PM +0800, Liping Zhang wrote:
> >> From: Liping Zhang <liping.zhang@spreadtrum.com>
> >>
> >> There's a similar problem in xt_NFLOG, and was fixed by commit 7643507fe8b5
> >> ("netfilter: xt_NFLOG: nflog-range does not truncate packets"). Only set
> >> copy_len here does not work, so we should enable NF_LOG_F_COPY_LEN also.
> >
> >Applied, thanks.
> >
> >Will you send me a patch for nftables userspace to enable this flag?
> >
> >It would be good to update the translation to make sure --nflog-size
> >map to snaplen and ignore --nflog-range.
>
> I find that nftables already support this feature, the following command mean to truncate packets
> to 100 bytes before logging to the userspace:
> #nft add rule filter input log group 0 snaplen 100
>
> Before my patch, it does not work.
> And after apply my patch, it works as expected.
If I git grep NF_LOG_F_COPY_LEN from the nftables.git tree, I don't
see any reference to this flag being set.
Then, nft_log kernel has been actually working fine since the
beginning. It would be good anyway if we set this flag on from
userspace to leave things in consistent state.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets
2016-07-20 8:25 ` Pablo Neira Ayuso
@ 2016-07-20 9:00 ` Liping Zhang
2016-07-20 9:07 ` Pablo Neira Ayuso
0 siblings, 1 reply; 11+ messages in thread
From: Liping Zhang @ 2016-07-20 9:00 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Liping Zhang, netfilter-devel, Liping Zhang
Hi Pablo,
2016-07-20 16:25 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> On Wed, Jul 20, 2016 at 07:00:13AM +0800, Liping Zhang wrote:
>> I find that nftables already support this feature, the following command mean to truncate packets
>> to 100 bytes before logging to the userspace:
>> #nft add rule filter input log group 0 snaplen 100
>>
>> Before my patch, it does not work.
>> And after apply my patch, it works as expected.
>
> If I git grep NF_LOG_F_COPY_LEN from the nftables.git tree, I don't
> see any reference to this flag being set.
>
I use this NF_LOG_F_COPY_LEN flag as internal, and when the user specify
the NFTA_LOG_SNAPLEN attrribute, it will be enabled automatically.
See my codes:
if (tb[NFTA_LOG_SNAPLEN] != NULL) {
+ li->u.ulog.flags |= NF_LOG_F_COPY_LEN;
- if (li->u.ulog.copy_len) {
+ if (li->u.ulog.flags & NF_LOG_F_COPY_LEN) {
if (nla_put_be32(skb, NFTA_LOG_SNAPLEN,
htonl(li->u.ulog.copy_len)))
So this flag will not be setted from userspace explicitly and will not
be dumped to the userspace.
> Then, nft_log kernel has been actually working fine since the
> beginning. It would be good anyway if we set this flag on from
> userspace to leave things in consistent state.
Do you mean this is something similar to "nflog-size" and
"nflog-range" in xt_NFLOG?
"nft log snaplen 100" cannot work since the beginning, so we should
keep it unchanged? And
maybe we should introduce a new option like "nft log snapsize 100"?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets
2016-07-20 9:00 ` Liping Zhang
@ 2016-07-20 9:07 ` Pablo Neira Ayuso
0 siblings, 0 replies; 11+ messages in thread
From: Pablo Neira Ayuso @ 2016-07-20 9:07 UTC (permalink / raw)
To: Liping Zhang; +Cc: Liping Zhang, netfilter-devel, Liping Zhang
On Wed, Jul 20, 2016 at 05:00:45PM +0800, Liping Zhang wrote:
> Hi Pablo,
>
> 2016-07-20 16:25 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> > On Wed, Jul 20, 2016 at 07:00:13AM +0800, Liping Zhang wrote:
> >> I find that nftables already support this feature, the following command mean to truncate packets
> >> to 100 bytes before logging to the userspace:
> >> #nft add rule filter input log group 0 snaplen 100
> >>
> >> Before my patch, it does not work.
> >> And after apply my patch, it works as expected.
> >
> > If I git grep NF_LOG_F_COPY_LEN from the nftables.git tree, I don't
> > see any reference to this flag being set.
> >
>
> I use this NF_LOG_F_COPY_LEN flag as internal, and when the user specify
> the NFTA_LOG_SNAPLEN attrribute, it will be enabled automatically.
> See my codes:
>
> if (tb[NFTA_LOG_SNAPLEN] != NULL) {
> + li->u.ulog.flags |= NF_LOG_F_COPY_LEN;
>
> - if (li->u.ulog.copy_len) {
> + if (li->u.ulog.flags & NF_LOG_F_COPY_LEN) {
> if (nla_put_be32(skb, NFTA_LOG_SNAPLEN,
> htonl(li->u.ulog.copy_len)))
>
> So this flag will not be setted from userspace explicitly and will not
> be dumped to the userspace.
That's fine the way it is then. Thanks for clarifying this.
> > Then, nft_log kernel has been actually working fine since the
> > beginning. It would be good anyway if we set this flag on from
> > userspace to leave things in consistent state.
>
> Do you mean this is something similar to "nflog-size" and
> "nflog-range" in xt_NFLOG?
> "nft log snaplen 100" cannot work since the beginning, so we should
> keep it unchanged? And
> maybe we should introduce a new option like "nft log snapsize 100"?
In the particular case of nftables, given we're still below the 1.0
stage, we can just classify this as a bugfix, I wouldn't bother much
on this.
The only thing to care is to provide the right translation of
nflog-size to snaplen and you actually contributed this patch already,
so everything is fine :).
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-07-20 9:07 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-18 12:44 [PATCH nf-next 0/3] netfilter: fix some small bugs related to nft_log Liping Zhang
2016-07-18 12:44 ` [PATCH nf-next 1/3] netfilter: nft_log: fix possible memory leak if log expr init fail Liping Zhang
2016-07-19 18:13 ` Pablo Neira Ayuso
2016-07-18 12:44 ` [PATCH nf-next 2/3] netfilter: nft_log: check the validity of log level Liping Zhang
2016-07-19 18:14 ` Pablo Neira Ayuso
2016-07-18 12:44 ` [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets Liping Zhang
2016-07-19 18:16 ` Pablo Neira Ayuso
2016-07-19 23:00 ` Liping Zhang
2016-07-20 8:25 ` Pablo Neira Ayuso
2016-07-20 9:00 ` Liping Zhang
2016-07-20 9:07 ` Pablo Neira Ayuso
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).