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: <20120627093325.GB410@vandijck-laurijssen.be>
Date:	Wed, 27 Jun 2012 11:33:25 +0200
From:	Kurt Van Dijck <kurt.van.dijck@....be>
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

Oliver, Rostislav,

I was just looking into this. I think the matching itself
may be simplified by eliminating checks 'that have already been made'.

On Tue, Jun 26, 2012 at 10:07:56PM +0200, Oliver Hartkopp wrote:
> 
> > +static int em_canid_match(struct sk_buff *skb, struct tcf_ematch *m,
> > +			 struct tcf_pkt_info *info)
> > +{
> > +	struct canid_match *cm = em_canid_priv(m);
> > +	canid_t can_id;
> > +	unsigned int match = false;
> > +	int i;
> > +
> > +	can_id = em_canid_get_id(skb);
> > +
> > +	if (can_id & CAN_EFF_FLAG) {
> > +		can_id &= CAN_EFF_MASK;
Why clear the CAN_EFF_FLAG?
It needs an extra read-modify-write, and the CAN_EFF_FLAG is set in
the match rule too.
IMHO, you could leave can_id as it is.
> > +
> > +		for (i = 0; i < cm->eff_rules_count; i++) {

to speed things up manually, I tend to use a 'const struct can_filter *lp'
and do:
		for (i = 0, lp = cm->rules_raw; i < cm->eff_rules_count;
				++i, ++lp) {
The advantage depends on the compiler's optimizations, which I'm not really
aware of.
The next statement would then be:

			if (!((lp->can_id ^ can_id) & lp->can_mask)) {

for stripping & CAN_EFF_MASK, see below :-)


> > +			if (!(((cm->rules_raw[i].can_id ^ can_id) &
> > +			    cm->rules_raw[i].can_mask) & CAN_EFF_MASK)) {
> 
> 
> Looks really tricky. I'm still pondering if it does what it should do ...

I think it does, since:
	cm->rules_raw[i].can_id ^ can_id
gives an value where the bits that differ are set.
then:
	& cm->rules_raw[i].can_mask
will strip bits that you don't care.

But '& CAN_EFF_MASK' is not needed, since:
	* can_id will have CAN_EFF_FLAG (see comment above)
	* cm->rules_raw[i].can_id has CAN_EFF_FLAG, otherwise it would
	  not be in the list
	* can_id will not have CAN_ERR_FLAG
	* cm->rules_raw should not have CAN_ERR_FLAG
	  you can always clear CAN_ERR_FLAG from the rules during
	  em_canid_change below
	* maybe distinguishing on CAN_RTR_FLAG makes sense during
	  classification.
> 
> > +				match = true;
> > +				break;
> > +			}
> > +		}
> > +	} else { /* SFF */
> > +		can_id &= CAN_SFF_MASK;
> > +		match = test_bit(can_id, cm->match_sff);
> > +	}
> > +
> 
> 
> return match;
> 
> 
> > +}
> > +
--
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