netdev - Re: [PATCH net-next v2 3/3] macsec: introduce IEEE 802.1AE driver

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] [day] [month] [year] [list]

Date:	Wed, 16 Mar 2016 10:50:39 +0100
From:	Sabrina Dubroca <sd@...asysnail.net>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	netdev@...r.kernel.org,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Florian Westphal <fw@...len.de>,
	Paolo Abeni <pabeni@...hat.com>, stephen@...workplumber.org
Subject: Re: [PATCH net-next v2 3/3] macsec: introduce IEEE 802.1AE driver

Hello,

2016-03-15, 15:03:31 +0100, Johannes Berg wrote:
> Hi,
> 
> > +struct macsec_rx_sa_stats {
> > +	__u32 InPktsOK;
> > +	__u32 InPktsInvalid;
> > +	__u32 InPktsNotValid;
> > +	__u32 InPktsNotUsingSA;
> > +	__u32 InPktsUnusedSA;
> > +};
> > +
> > +struct macsec_tx_sa_stats {
> > +	__u32 OutPktsProtected;
> > +	__u32 OutPktsEncrypted;
> > +};
> 
> Just noticed this: is there a particular reason for using only __u32
> here? The others all seem to use __u64.

The standard defines them this way (Counter32 for the SA stats,
Counter64 for the SC/SecY stats).


> > +static int macsec_dump_txsc(struct sk_buff *skb, struct
> > netlink_callback *cb)
> > +{
> > +	struct net *net = sock_net(skb->sk);
> > +	struct net_device *dev;
> > +	int dev_idx, d;
> > +
> > +	dev_idx = cb->args[0];
> > +
> > +	d = 0;
> > +	for_each_netdev(net, dev) {
> > +		struct macsec_secy *secy;
> > +
> > +		if (d < dev_idx)
> > +			goto next;
> > +
> > +		if (!netif_is_macsec(dev))
> > +			goto next;
> > +
> > +		secy = &macsec_priv(dev)->secy;
> > +		if (dump_secy(secy, dev, skb, cb) < 0)
> > +			goto done;
> > +next:
> > +		d++;
> > +	}
> > +
> > +done:
> > +	cb->args[0] = d;
> > +	return skb->len;
> > +}
> 
> Maybe you should consider adding genl_dump_check_consistent() support
> here, so userspace can figure out if the dump was really consistent, if
> necessary.
> 
> To do this, you have to keep a global generation counter that changes
> whenever this list changes (adding/removing macsec interfaces, I think)
> and then set
> 
> 	cb->seq = macsec_generation_counter;
> 
> at the beginning of this function, and call
> genl_dump_check_consistent() for each message in the loop.

Thanks, I'll have a look at this.


> Btw, aren't you missing locking here for for_each_netdev()?

Oops, yeah.

-- 
Sabrina