netdev - Re: [RFC PATCH net-next 10/12] drop_monitor: Add packet alert mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190724165725.GC20252@splinter>
Date:   Wed, 24 Jul 2019 19:57:25 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, nhorman@...driver.com,
        dsahern@...il.com, roopa@...ulusnetworks.com,
        nikolay@...ulusnetworks.com, jakub.kicinski@...ronome.com,
        toke@...hat.com, andy@...yhouse.net, f.fainelli@...il.com,
        andrew@...n.ch, vivien.didelot@...il.com, mlxsw@...lanox.com,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [RFC PATCH net-next 10/12] drop_monitor: Add packet alert mode

On Wed, Jul 24, 2019 at 02:53:41PM +0200, Jiri Pirko wrote:
> Mon, Jul 22, 2019 at 08:31:32PM CEST, idosch@...sch.org wrote:
> >+static const struct net_dm_alert_ops *net_dm_alert_ops_arr[] = {
> >+	[NET_DM_ALERT_MODE_SUMMARY]	= &net_dm_alert_summary_ops,
> >+	[NET_DM_ALERT_MODE_PACKET]	= &net_dm_alert_packet_ops,
> >+};
> 
> Please split this patch into 2:
> 1) introducing the ops and modes (only summary)
> 2) introducing the packet mode

Ack

...

> >+static int net_dm_alert_mode_set(struct genl_info *info)
> >+{
> >+	struct netlink_ext_ack *extack = info->extack;
> >+	enum net_dm_alert_mode alert_mode;
> >+	int rc;
> >+
> >+	if (!info->attrs[NET_DM_ATTR_ALERT_MODE])
> >+		return 0;
> >+
> >+	rc = net_dm_alert_mode_get_from_info(info, &alert_mode);
> >+	if (rc) {
> >+		NL_SET_ERR_MSG_MOD(extack, "Invalid alert mode");
> >+		return -EINVAL;
> >+	}
> >+
> >+	net_dm_alert_mode = alert_mode;
> 
> 2 things:
> 1) Shouldn't you check if the tracing is on and return -EBUSY in case it is?

I'm doing it below in net_dm_cmd_config() :)
But I'm returning '-EOPNOTSUPP'. I guess '-EBUSY' is more appropriate.
Will change.

> 2) You setup the mode globally. I guess it is fine and it does not make
>    sense to do it otherwise, right? Like per-net or something.

Yes, it's global. I didn't change that aspect of drop monitor and I
don't really see a use case for that.

> 
> 
> >+
> >+	return 0;
> >+}
> >+
> > static int net_dm_cmd_config(struct sk_buff *skb,
> > 			struct genl_info *info)
> > {
> >-	NL_SET_ERR_MSG_MOD(info->extack, "Command not supported");
> >+	struct netlink_ext_ack *extack = info->extack;
> >+	int rc;
> > 
> >-	return -EOPNOTSUPP;
> >+	if (trace_state == TRACE_ON) {
> >+		NL_SET_ERR_MSG_MOD(extack, "Cannot configure drop monitor while tracing is on");
> >+		return -EOPNOTSUPP;
> >+	}
> >+
> >+	rc = net_dm_alert_mode_set(info);
> >+	if (rc)
> >+		return rc;
> >+
> >+	return 0;
> > }