netdev - Re: [RFC 5/9] snet: introduce snet_event.c and snet

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: <m2ws00oz6a.fsf@ssh.synack.fr>
Date:	Sun, 03 Jan 2010 00:38:21 +0100
From:	Samir Bellabes <sam@...ack.fr>
To:	Evgeniy Polyakov <zbr@...emap.net>
Cc:	linux-security-module@...r.kernel.org,
	Patrick McHardy <kaber@...sh.net>, jamal <hadi@...erus.ca>,
	Neil Horman <nhorman@...driver.com>, netdev@...r.kernel.org,
	netfilter-devel@...r.kernel.org
Subject: Re: [RFC 5/9] snet: introduce snet_event.c and snet_event.h

Hello Evgeniy,

Evgeniy Polyakov <zbr@...emap.net> writes:
>> +
>> +extern unsigned int event_hash_size;
>> +
>
>> +
>> +static struct list_head *event_hash;
>> +static rwlock_t event_hash_lock = __RW_LOCK_UNLOCKED();
>> +
>
> Those are way too generic names, please use snet_ prefix as you did in
> other places.

done. thank you

> I believe snet should be converted to RCU instead of rw locks.

I see, I will investigated this idea.

>> +/* lookup for a event_hash - before using this function, lock event_hash_lock */
>> +static struct snet_event_entry *__snet_event_lookup(const enum snet_syscall syscall,
>> +						   const u8 protocol)
>> +{
>> +	unsigned int h = 0;
>> +	struct list_head *l;
>> +	struct snet_event_entry *s;
>> +	struct snet_event t;
>> +
>> +	if (!event_hash)
>> +		return NULL;
>> +
>> +	/* building the element to look for */
>> +	t.syscall = syscall;
>> +	t.protocol = protocol;
>> +
>> +	/* computing its hash value */
>> +	h = jhash(&t, sizeof(struct snet_event), 0) % event_hash_size;
>
> You can have better distribution if not simply cut off the remainder.
> Also it is a rather expensive operation.

true, I replaced the modulo operation to a bitmask operations.

>> +	l = &event_hash[h];
>> +
>> +	list_for_each_entry(s, l, list) {
>> +		if ((s->se.protocol == protocol) &&
>> +		    (s->se.syscall == syscall)) {
>> +			return s;
>> +		}
>> +	}
>> +	return NULL;
>> +}
>
> Below comments looks a little bit weird, was it generated automatically
> by editor?

no, it's was a unused debug function. I deleted this comment.

>> +/* void snet_event_dumpall() */
>> +/* { */
>> +/*	unsigned int i = 0; */
>> +/*	struct list_head *l; */
>> +/*	struct snet_event_entry *s; */
>> +
>> +/*	snet_dbg("entering\n"); */
>> +/*	read_lock_bh(&event_hash_lock); */
>> +/*	for (i = 0; i < (event_hash_size - 1); i++) { */
>> +/*		l = &hash[i]; */
>> +/*		list_for_each_entry(s, l, list) { */
>> +/*			snet_dbg("[%d, %d, %d]\n", i, */
>> +/*				 s->se.protocol, s->se.syscall); */
>> +/*		} */
>> +/*	} */
>> +/*	read_unlock_bh(&event_hash_lock); */
>> +/*	snet_dbg("exiting\n"); */
>> +/*	return; */
>> +/* } */
>
>> +int snet_event_insert(const enum snet_syscall syscall, const u8 protocol)
>> +{
>> +	struct snet_event_entry *data = NULL;
>> +	unsigned int h = 0;
>> +
>> +	data = kzalloc(sizeof(struct snet_event_entry), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	write_lock_bh(&event_hash_lock);
>> +	/* check if event is already registered */
>> +	if (!event_hash || __snet_event_lookup(syscall, protocol) != NULL) {
>> +		write_unlock_bh(&event_hash_lock);
>> +		kfree(data);
>> +		return -EINVAL;
>> +	}
>
> What about single error exiting point?

done, thanks

>> +
>> +	data->se.syscall = syscall;
>> +	data->se.protocol = protocol;
>> +	INIT_LIST_HEAD(&(data->list));
>> +	h = jhash(&(data->se), sizeof(struct snet_event), 0) % event_hash_size;
>> +	list_add_tail(&data->list, &event_hash[h]);
>> +	write_unlock_bh(&event_hash_lock);
>> +
>> +	return 0;
>> +}
>
>> +/* init function */
>> +int snet_event_init(void)
>> +{
>> +	int err = 0, i = 0;
>> +
>> +	event_hash = kzalloc(sizeof(struct list_head) * event_hash_size,
>> +			     GFP_KERNEL);
>> +	if (!event_hash) {
>> +		printk(KERN_WARNING
>> +		       "snet: can't alloc memory for event_hash\n");
>> +		err = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	for (i = 0; i < event_hash_size; i++)
>> +		INIT_LIST_HEAD(&(event_hash[i]));
>
> No need for double braces.

again fixed.

thanks for your time Evgeniy,
sam
--
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