[nft PATCH] monitor: Print NEWGEN events

[nft PATCH] monitor: Print NEWGEN events

[Phil Sutter]

Now that they contain process information, they're actually interesting.
For backwards compatibility, print process information only if it was
present in the message.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/linux/netfilter/nf_tables.h |  2 ++
 src/netlink.c                       | 40 +++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index 683f6f88fcace..40096de04e963 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -1221,6 +1221,8 @@ enum nft_objref_attributes {
 enum nft_gen_attributes {
 	NFTA_GEN_UNSPEC,
 	NFTA_GEN_ID,
+	NFTA_GEN_PROC_ID,
+	NFTA_GEN_PROC_NAME,
 	__NFTA_GEN_MAX
 };
 #define NFTA_GEN_MAX		(__NFTA_GEN_MAX - 1)
diff --git a/src/netlink.c b/src/netlink.c
index e3c90dac8c7a6..44a9806097d00 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -2915,6 +2915,43 @@ static void netlink_events_debug(uint16_t type)
 #endif /* DEBUG */
 }
 
+static int netlink_events_newgen_cb(const struct nlmsghdr *nlh, int type,
+				    struct netlink_mon_handler *monh)
+{
+	const struct nlattr *attr;
+	char name[256] = "";
+	int genid, pid = -1;
+
+	mnl_attr_for_each(attr, nlh, sizeof(struct nfgenmsg)) {
+		switch (mnl_attr_get_type(attr)) {
+		case NFTA_GEN_ID:
+			if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
+				break;
+			genid = ntohl(mnl_attr_get_u32(attr));
+			break;
+		case NFTA_GEN_PROC_NAME:
+			if (mnl_attr_validate(attr, MNL_TYPE_NUL_STRING) < 0)
+				break;
+			strncpy(name, mnl_attr_get_str(attr), sizeof(name));
+			break;
+		case NFTA_GEN_PROC_ID:
+			if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
+				break;
+			pid = ntohl(mnl_attr_get_u32(attr));
+			break;
+		}
+	}
+	printf("new generation %d", genid);
+	if (pid >= 0) {
+		printf(" by process %d", pid);
+		if (!monh->ctx->octx->numeric)
+			printf(" (%s)", name);
+	}
+	printf("\n");
+
+	return MNL_CB_OK;
+}
+
 static int netlink_events_cb(const struct nlmsghdr *nlh, void *data)
 {
 	int ret = MNL_CB_OK;
@@ -2955,6 +2992,9 @@ static int netlink_events_cb(const struct nlmsghdr *nlh, void *data)
 	case NFT_MSG_DELOBJ:
 		ret = netlink_events_obj_cb(nlh, type, monh);
 		break;
+	case NFT_MSG_NEWGEN:
+		ret = netlink_events_newgen_cb(nlh, type, monh);
+		break;
 	}
 	fflush(stdout);
 
-- 
2.13.1

Re: [nft PATCH] monitor: Print NEWGEN events

[Pablo Neira Ayuso]

On Wed, Jul 19, 2017 at 04:32:57PM +0200, Phil Sutter wrote:
> Now that they contain process information, they're actually interesting.
> For backwards compatibility, print process information only if it was
> present in the message.

Also applied, thanks.

Re: [nft PATCH] monitor: Print NEWGEN events

[Pablo Neira Ayuso]

On Wed, Jul 19, 2017 at 04:32:57PM +0200, Phil Sutter wrote:
> Now that they contain process information, they're actually interesting.
> For backwards compatibility, print process information only if it was
> present in the message.

Wait, a couple of comments.

[...]
> diff --git a/src/netlink.c b/src/netlink.c
> index e3c90dac8c7a6..44a9806097d00 100644
> --- a/src/netlink.c
> +++ b/src/netlink.c
> @@ -2915,6 +2915,43 @@ static void netlink_events_debug(uint16_t type)
>  #endif /* DEBUG */
>  }
>  
> +static int netlink_events_newgen_cb(const struct nlmsghdr *nlh, int type,
> +				    struct netlink_mon_handler *monh)
> +{
> +	const struct nlattr *attr;
> +	char name[256] = "";
> +	int genid, pid = -1;
> +
> +	mnl_attr_for_each(attr, nlh, sizeof(struct nfgenmsg)) {
> +		switch (mnl_attr_get_type(attr)) {
> +		case NFTA_GEN_ID:
> +			if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
> +				break;

I think it's better to hit netlink_abi_error() error. If validation
fails, it means someone in the kernel has accidentally changed the
size/type of this attribute. This it's basically breaking the ABI.
Better bail out here so people come to us so we quickly fix this.

> +			genid = ntohl(mnl_attr_get_u32(attr));
> +			break;
> +		case NFTA_GEN_PROC_NAME:
> +			if (mnl_attr_validate(attr, MNL_TYPE_NUL_STRING) < 0)
> +				break;
> +			strncpy(name, mnl_attr_get_str(attr), sizeof(name));

What is maximum process name length? If we hit this bound, we have to
make sure this does:

                        name[X - 1] = '\0';

Where X is the name buffer size.

> +			break;
> +		case NFTA_GEN_PROC_ID:
> +			if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
> +				break;
> +			pid = ntohl(mnl_attr_get_u32(attr));
> +			break;
> +		}
> +	}
> +	printf("new generation %d", genid);

Please, prepend '#' to this message.

> +	if (pid >= 0) {
> +		printf(" by process %d", pid);
> +		if (!monh->ctx->octx->numeric)
> +			printf(" (%s)", name);
> +	}
> +	printf("\n");
> +
> +	return MNL_CB_OK;
> +}

Thanks!

Re: [nft PATCH] monitor: Print NEWGEN events

[Phil Sutter]

On Mon, Jul 24, 2017 at 01:17:30PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Jul 19, 2017 at 04:32:57PM +0200, Phil Sutter wrote:
> > Now that they contain process information, they're actually interesting.
> > For backwards compatibility, print process information only if it was
> > present in the message.
> 
> Wait, a couple of comments.

All ACK, one remark:

[...]
> > +		case NFTA_GEN_PROC_NAME:
> > +			if (mnl_attr_validate(attr, MNL_TYPE_NUL_STRING) < 0)
> > +				break;
> > +			strncpy(name, mnl_attr_get_str(attr), sizeof(name));
> 
> What is maximum process name length? If we hit this bound, we have to
> make sure this does:
> 
>                         name[X - 1] = '\0';
> 
> Where X is the name buffer size.

NFTA_GEN_PROC_NAME attribute is filled with output from get_task_comm(),
which returns a string of max 16 bytes length. It is safe to assume that
it's NULL terminated since set_task_comm() uses strlcpy().

That static buffer above is needless though, so I'll change it to just
point to the netlink attribute itself if it is present.

Thanks, Phil