netdev - Re: [PATCH net-next] em_canid: Ematch rule to match CAN frames according to their CAN IDs

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: <20120626213255.GC31808@canuck.infradead.org>
Date:	Tue, 26 Jun 2012 17:32:55 -0400
From:	Thomas Graf <tgraf@...g.ch>
To:	Oliver Hartkopp <socketcan@...tkopp.net>
Cc:	Rostislav Lisovy <lisovy@...il.com>,
	Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org,
	linux-can@...r.kernel.org, lartc@...r.kernel.org,
	pisa@....felk.cvut.cz, sojkam1@....cvut.cz
Subject: Re: [PATCH net-next] em_canid: Ematch rule to match CAN frames
 according to their CAN IDs

On Tue, Jun 26, 2012 at 10:07:56PM +0200, Oliver Hartkopp wrote:
> > With no extra filter/qdisc configured, median of the time spent in can_send()
> > was about 27 us -- with prio qdisc with 5 bands and 5 appropriate cls_can
> > filters (previous patch), it was about 30 us -- with prio qdisc with 5 bands
> > and 5 appropriate em_can filters (this patch), it was about 34 us.
> 
> 
> Hm that's more than twice the time consumed for classification ...
> 
> cls_can: 3 us more
> em_can:  7 us more
> 
> @Eric: Is this still the better approach then?

If there is overhead, we should get rid of that overhead and not
abandon an established subsystem.

Rostislav: Can you provide some details on where the time is spent?

> > +	/* Process EFF frame rules*/
> > +	for (i = 0; i < cm->rules_count; i++) {
> > +		if ((conf[i].can_id & CAN_EFF_FLAG) &&
> > +		    (conf[i].can_mask & CAN_EFF_FLAG)) {
> > +			memcpy(cm->rules_raw + cm->eff_rules_count,
> 
> 
> Oops. Shouldn't this be
> 
> cm->rules_raw + cm->eff_rules_count * sizeof(struct can_filter),
> 
> ???

Looks like correct pointer arithmetic to me. Your suggestion
would only be valid if rules_raw was a void pointer.

> > +static void em_canid_destroy(struct tcf_proto *tp, struct tcf_ematch *m)
> > +{
> > +	struct canid_match *cm = em_canid_priv(m);
> > +
> 
> 
> Check for cm == NULL not needed ?

kfree() has that check embeddded. Also, for destroy() can only be called
if the match was added to the tree and that requires a successful call
to ->change(). Therefore it will never be NULL.

> > +static int em_canid_dump(struct sk_buff *skb, struct tcf_ematch *m)
> > +{
> > +	struct canid_match *cm = em_canid_priv(m);
> > +
> 
> 
> Check for cm == NULL not needed ?
> 
> Can a dump happen before the matches are added??

Nope, ->dump() is only ever called if the match has been added to the tree.

> > +	/*
> > +	 * When configuring this ematch 'rules_count' is set not to exceed
> > +	 * 'rules_raw' array size
> > +	 */
> > +	if (nla_put_nohdr(skb, sizeof(cm->rules_raw[0]) * cm->rules_count,
> 
> 
> better sizeof(struct can_filter) instead of sizeof(cm->rules_raw[0]) ??
> 
> > +	    &cm->rules_raw) < 0)
> > +		goto nla_put_failure;

No need for a goto here, just return -EMSGSIZE.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html