nftables: fix surpression of "permission denied" errors
RFC PATCH 1/1 nftables: fix surpression of "permission denied" errors
commit 8ca6730e9c8fd59d8a03ae505777a8a6b97898b1
Author: Patrick McHardy <[email protected]> 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.
include/mnl.h +1 -1
@@ -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,
src/main.c +8 -4
@@ -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 ; + } } } }
src/mnl.c +5 -3
@@ -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 )
Pablo Neira Ayuso
maintainerHi Patrick,
Quoted 12 lines · Patrick McHardy
commit
8ca6730e9c8fd59d8a03ae505777a8a6b97898b1Author: Patrick McHardy <[email protected]> Date: Wed Jan 8 20:57:11 2014 +0000nftables: 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_BEGINmessage instead of the command messages. Also only a single error message is generated for the entire batch.
Quoted 3 lines
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!
Quoted 15 lines · Pablo Neira Ayuso
Hi Patrick,
Quoted 12 lines · Patrick McHardy
commit
8ca6730e9c8fd59d8a03ae505777a8a6b97898b1Author: Patrick McHardy <[email protected]> Date: Wed Jan 8 20:57:11 2014 +0000nftables: 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_BEGINmessage instead of the command messages. Also only a single error message is generated for the entire batch.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 5 lines
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:
Quoted 7 lines
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.
Pablo Neira Ayuso
maintainerQuoted 7 lines · Patrick McHardy
Quoted 4 lines · Pablo Neira Ayuso
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.
Quoted 8 lines
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.
Quoted 8 lines
Quoted 8 lines
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)
Quoted 7 lines
Quoted 4 lines
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.
Quoted 4 lines
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 ofnetlink_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.
Quoted 17 lines · Pablo Neira Ayuso
Quoted 7 lines · Patrick McHardy
Quoted 4 lines · Pablo Neira Ayuso
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.
Quoted 8 lines
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.
Quoted 2 lines
Let's do that, this case is very specific of EPERM. Please, push it to master.
Will do.
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
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.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 11 lines
Quoted 4 lines
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 ofnetlink_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.
Quoted 5 lines
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 :)