[PATCH RFC] nftables: fix surpression of "permission denied" errors

[PATCH RFC] nftables: fix surpression of "permission denied" errors

[Patrick McHardy]

commit 8ca6730e9c8fd59d8a03ae505777a8a6b97898b1
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed Jan 8 20:57:11 2014 +0000

    nftables: fix surpression of "permission denied" errors
    
    ntroduction of batch support broke displaying of EPERM since those are
    generated by the kernel before batch processing start and thus have the
    sequence number of the NFNL_MSG_BATCH_BEGIN message instead of the
    command messages. Also only a single error message is generated for the
    entire batch.
    
    This patch fixes this by noting the batch sequence number and displaying
    the error for all commands since this is what would happen if the
    permission check was inside batch processing as every other check.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/include/mnl.h b/include/mnl.h
index fe2fb40..a630605 100644
--- a/include/mnl.h
+++ b/include/mnl.h
@@ -18,7 +18,7 @@ void mnl_err_list_free(struct mnl_err *err);
 void mnl_batch_init(void);
 bool mnl_batch_ready(void);
 void mnl_batch_reset(void);
-void mnl_batch_begin(void);
+uint32_t mnl_batch_begin(void);
 void mnl_batch_end(void);
 int mnl_batch_talk(struct mnl_socket *nl, struct list_head *err_list);
 int mnl_nft_rule_batch_add(struct nft_rule *nlr, unsigned int flags,
diff --git a/src/main.c b/src/main.c
index 859ddaa..33a02e1 100644
--- a/src/main.c
+++ b/src/main.c
@@ -160,9 +160,10 @@ static int nft_netlink(struct parser_state *state, struct list_head *msgs)
 	struct cmd *cmd, *next;
 	struct mnl_err *err, *tmp;
 	LIST_HEAD(err_list);
+	uint32_t batch_seqnum;
 	int ret = 0;
 
-	mnl_batch_begin();
+	batch_seqnum = mnl_batch_begin();
 	list_for_each_entry(cmd, &state->cmds, list) {
 		memset(&ctx, 0, sizeof(ctx));
 		ctx.msgs = msgs;
@@ -183,12 +184,15 @@ static int nft_netlink(struct parser_state *state, struct list_head *msgs)
 
 	list_for_each_entry_safe(err, tmp, &err_list, head) {
 		list_for_each_entry(cmd, &state->cmds, list) {
-			if (err->seqnum == cmd->seqnum) {
+			if (err->seqnum == cmd->seqnum ||
+			    err->seqnum == batch_seqnum) {
 				netlink_io_error(&ctx, &cmd->location,
 					"Could not process rule in batch: %s",
 					strerror(err->err));
-				mnl_err_list_free(err);
-				break;
+				if (err->seqnum == cmd->seqnum) {
+					mnl_err_list_free(err);
+					break;
+				}
 			}
 		}
 	}
diff --git a/src/mnl.c b/src/mnl.c
index a711b5e..a4a4c4a 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -106,7 +106,7 @@ static void mnl_batch_page_add(void)
 	batch = mnl_batch_alloc();
 }
 
-static void mnl_batch_put(int type)
+static uint32_t mnl_batch_put(int type)
 {
 	struct nlmsghdr *nlh;
 	struct nfgenmsg *nfg;
@@ -123,11 +123,13 @@ static void mnl_batch_put(int type)
 
 	if (!mnl_nlmsg_batch_next(batch))
 		mnl_batch_page_add();
+
+	return nlh->nlmsg_seq;
 }
 
-void mnl_batch_begin(void)
+uint32_t mnl_batch_begin(void)
 {
-	mnl_batch_put(NFNL_MSG_BATCH_BEGIN);
+	return mnl_batch_put(NFNL_MSG_BATCH_BEGIN);
 }
 
 void mnl_batch_end(void)

Re: [PATCH RFC] nftables: fix surpression of "permission denied" errors

[Pablo Neira Ayuso]

Hi Patrick,

On Wed, Jan 08, 2014 at 08:58:13PM +0000, Patrick McHardy wrote:
> commit 8ca6730e9c8fd59d8a03ae505777a8a6b97898b1
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Wed Jan 8 20:57:11 2014 +0000
> 
>     nftables: fix surpression of "permission denied" errors
>     
>     ntroduction of batch support broke displaying of EPERM since those are
>     generated by the kernel before batch processing start and thus have the
>     sequence number of the NFNL_MSG_BATCH_BEGIN message instead of the
>     command messages. Also only a single error message is generated for the
>     entire batch.
>     
>     This patch fixes this by noting the batch sequence number and displaying
>     the error for all commands since this is what would happen if the
>     permission check was inside batch processing as every other check.

One error message per line can be too much if we have a big batch,
perhaps we can just point to the first rule in the batch and print
something like: "7 error suppressed.", similar to syslog, where 7 is
the number of rules that follow up after EPERM message.

BTW, with really really big batches, the kernel may fail to allocate
the acknowledgment. We discussed this already with David, and he
thinks it doesn't make sense to send such a big message back to
sender. We can add:

 void netlink_ack_len(struct sk_buff *in_skb, struct nlmsghdr *nlh,
                      int err, int len)

where len specifies the length would be the original netlink header +
nfnetlink header, so the rules are not sent back to userspace.

Let me know, thanks!

Re: [PATCH RFC] nftables: fix surpression of "permission denied" errors

[Patrick McHardy]

On Thu, Jan 09, 2014 at 06:40:25PM +0100, Pablo Neira Ayuso wrote:
> Hi Patrick,
> 
> On Wed, Jan 08, 2014 at 08:58:13PM +0000, Patrick McHardy wrote:
> > commit 8ca6730e9c8fd59d8a03ae505777a8a6b97898b1
> > Author: Patrick McHardy <kaber@trash.net>
> > Date:   Wed Jan 8 20:57:11 2014 +0000
> > 
> >     nftables: fix surpression of "permission denied" errors
> >     
> >     ntroduction of batch support broke displaying of EPERM since those are
> >     generated by the kernel before batch processing start and thus have the
> >     sequence number of the NFNL_MSG_BATCH_BEGIN message instead of the
> >     command messages. Also only a single error message is generated for the
> >     entire batch.
> >     
> >     This patch fixes this by noting the batch sequence number and displaying
> >     the error for all commands since this is what would happen if the
> >     permission check was inside batch processing as every other check.
> 
> One error message per line can be too much if we have a big batch,
> perhaps we can just point to the first rule in the batch and print
> something like: "7 error suppressed.", similar to syslog, where 7 is
> the number of rules that follow up after EPERM message.

Well, this was done to stay consistent with any other error type.
We could compress similar errors, but there are a few things to
consider. One is that it might be hard to point to the correct
spot(s) in the error message. Also the output format was chosen to
be similar to gcc so f.i. vi could easily jump to rules causing
errors in a file. For now I think we should apply this patch, which
will treat EPERM like any other error, and then work on something
to reduce the output.

> BTW, with really really big batches, the kernel may fail to allocate
> the acknowledgment. We discussed this already with David, and he
> thinks it doesn't make sense to send such a big message back to
> sender. We can add:
> 
>  void netlink_ack_len(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>                       int err, int len)
> 
> where len specifies the length would be the original netlink header +
> nfnetlink header, so the rules are not sent back to userspace.
> 
> Let me know, thanks!

Why would it depend on the size of the batch? The errors are allocated
for every single message *within* the batch. I agree though that if
we can allocate a smaller message without the full contents, this
is still better than using sk_err. But I guess that should simply
be fallback behaviour of netlink_ack() instead of specifying a
length.

Re: [PATCH RFC] nftables: fix surpression of "permission denied" errors

[Pablo Neira Ayuso]

On Thu, Jan 09, 2014 at 06:01:41PM +0000, Patrick McHardy wrote:
> On Thu, Jan 09, 2014 at 06:40:25PM +0100, Pablo Neira Ayuso wrote:

> > One error message per line can be too much if we have a big batch,
> > perhaps we can just point to the first rule in the batch and print
> > something like: "7 error suppressed.", similar to syslog, where 7 is
> > the number of rules that follow up after EPERM message.
> 
> Well, this was done to stay consistent with any other error type.
> We could compress similar errors, but there are a few things to
> consider. One is that it might be hard to point to the correct
> spot(s) in the error message. Also the output format was chosen to
> be similar to gcc so f.i. vi could easily jump to rules causing
> errors in a file. For now I think we should apply this patch, which
> will treat EPERM like any other error, and then work on something
> to reduce the output.

Let's do that, this case is very specific of EPERM. Please, push it to
master.

> > BTW, with really really big batches, the kernel may fail to allocate
> > the acknowledgment. We discussed this already with David, and he
> > thinks it doesn't make sense to send such a big message back to
> > sender. We can add:
> > 
> >  void netlink_ack_len(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> >                       int err, int len)
> > 
> > where len specifies the length would be the original netlink header +
> > nfnetlink header, so the rules are not sent back to userspace.
> > 
> > Let me know, thanks!
> 
> Why would it depend on the size of the batch? The errors are allocated
> for every single message *within* the batch.

I was refering to the EPERM case and NFNL_MSG_BATCH_BEGIN.

> I agree though that if we can allocate a smaller message without the
> full contents, this is still better than using sk_err. But I guess
> that should simply be fallback behaviour of netlink_ack() instead of
> specifying a length.

Jamal mentioned that capping can be a possibility (like in ICMP
errors), but I think David prefered to remove the entire payload. The
sequence number already refers to the original message so we can
correlate it with what we have sent. Perhaps netlink_ack() can default
to that behaviour if it fails to allocate the skbuff, ie. try to
allocate a small one just containing the netlink error header.

However, the current approach, the length of the begin message is just
the netlink header + nfnetlink header, so we're not getting a large
skbuff back in this error case either, so I don't find any path we can
get big ack messages in nfnetlink at this moment, we can archive this
problem.

Re: [PATCH RFC] nftables: fix surpression of "permission denied" errors

[Patrick McHardy]

On Thu, Jan 09, 2014 at 07:48:21PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 09, 2014 at 06:01:41PM +0000, Patrick McHardy wrote:
> > On Thu, Jan 09, 2014 at 06:40:25PM +0100, Pablo Neira Ayuso wrote:
> 
> > > One error message per line can be too much if we have a big batch,
> > > perhaps we can just point to the first rule in the batch and print
> > > something like: "7 error suppressed.", similar to syslog, where 7 is
> > > the number of rules that follow up after EPERM message.
> > 
> > Well, this was done to stay consistent with any other error type.
> > We could compress similar errors, but there are a few things to
> > consider. One is that it might be hard to point to the correct
> > spot(s) in the error message. Also the output format was chosen to
> > be similar to gcc so f.i. vi could easily jump to rules causing
> > errors in a file. For now I think we should apply this patch, which
> > will treat EPERM like any other error, and then work on something
> > to reduce the output.
> 
> Let's do that, this case is very specific of EPERM. Please, push it to
> master.

Will do.

> > > BTW, with really really big batches, the kernel may fail to allocate
> > > the acknowledgment. We discussed this already with David, and he
> > > thinks it doesn't make sense to send such a big message back to
> > > sender. We can add:
> > > 
> > >  void netlink_ack_len(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> > >                       int err, int len)
> > > 
> > > where len specifies the length would be the original netlink header +
> > > nfnetlink header, so the rules are not sent back to userspace.
> > > 
> > > Let me know, thanks!
> > 
> > Why would it depend on the size of the batch? The errors are allocated
> > for every single message *within* the batch.
> 
> I was refering to the EPERM case and NFNL_MSG_BATCH_BEGIN.

Right. The easy solution would be to move the capable() check to
within batch processing. Processing NFNL_MSG_BATCH_BEGIN obviously
doesn't require special permissions.

> > I agree though that if we can allocate a smaller message without the
> > full contents, this is still better than using sk_err. But I guess
> > that should simply be fallback behaviour of netlink_ack() instead of
> > specifying a length.
> 
> Jamal mentioned that capping can be a possibility (like in ICMP
> errors), but I think David prefered to remove the entire payload. The
> sequence number already refers to the original message so we can
> correlate it with what we have sent. Perhaps netlink_ack() can default
> to that behaviour if it fails to allocate the skbuff, ie. try to
> allocate a small one just containing the netlink error header.

Yes, that's what I meant. This should of course only be the fallback
after failing to allocate the entire message and before resorting to
sk_err. I still dream of being able to pass the offset of the
attribute triggering an error back to userspace to very precisely
point to the error.

> However, the current approach, the length of the begin message is just
> the netlink header + nfnetlink header, so we're not getting a large
> skbuff back in this error case either, so I don't find any path we can
> get big ack messages in nfnetlink at this moment, we can archive this
> problem.

Ok perfect, so no changes required at all :)