Patchwork [RFC,4/9] snet: introduce snet_core.c and snet.h

login
register
mail settings
Submitter Samir Bellabes
Date Jan. 6, 2010, 7:46 p.m.
Message ID <m2y6kbm2xw.fsf@ssh.synack.fr>
Download mbox | patch
Permalink /patch/42323/
State RFC
Delegated to: David Miller
Headers show

Comments

Samir Bellabes - Jan. 6, 2010, 7:46 p.m.
Patrick McHardy <kaber@trash.net> writes:

>> +struct snet_event {
>> +	enum snet_syscall syscall;
>> +	u8 protocol;
>> +} __attribute__ ((packed));
>
> Does this really need to be packed? You're using it in a
> struct snet_event_entry, which is padded anyways.

I think that that members needs to be aligned, because this struct is
used for the jhash() computation.
when testing on x86_64, I discovered that jhash value wasn't the same,
for a same struct snet_event. Then I thougth about misaligned data, and
possible 'hole' between syscall and protocol.

anyway, I patched the code to use jhash_2words() or jhash_1word().
here is a patch, which apply on top of initial serie

thank you Patrick,
sam

commit bacdb6b62549b58370298f89f908d66c5a3cab66
Author: Samir Bellabes <sam@synack.fr>
Date:   Wed Jan 6 20:36:06 2010 +0100

    snet : delete attribute packed for snet_event and use proper jash functions
    
    the structure snet_event was using the attribute packed, which seems to be
    unnecessary.
    this patch also fix the good compute of hashes, by moving jash() to
    jash_1word() and jash_2words()
    
    Noticed by Patrick McHardy <kaber@trash.net>
    
    Signed-off-by: Samir Bellabes <sam@synack.fr>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Evgeniy Polyakov - Jan. 6, 2010, 7:58 p.m.
On Wed, Jan 06, 2010 at 08:46:35PM +0100, Samir Bellabes (sam@synack.fr) wrote:
> Patrick McHardy <kaber@trash.net> writes:
> 
> >> +struct snet_event {
> >> +	enum snet_syscall syscall;
> >> +	u8 protocol;
> >> +} __attribute__ ((packed));
> >
> > Does this really need to be packed? You're using it in a
> > struct snet_event_entry, which is padded anyways.
> 
> I think that that members needs to be aligned, because this struct is
> used for the jhash() computation.
> when testing on x86_64, I discovered that jhash value wasn't the same,
> for a same struct snet_event. Then I thougth about misaligned data, and
> possible 'hole' between syscall and protocol.

Without padding it will eat additional bytes after 'protocol' field,
since enum is 32-bit long.

> anyway, I patched the code to use jhash_2words() or jhash_1word().
> here is a patch, which apply on top of initial serie

What's the purpose of hashing verdict_id?
Samir Bellabes - Jan. 23, 2010, 2:07 a.m.
I'm sorry for the delay I missed this one.

Evgeniy Polyakov <zbr@ioremap.net> writes:
> On Wed, Jan 06, 2010 at 08:46:35PM +0100, Samir Bellabes (sam@synack.fr) wrote:
>> Patrick McHardy <kaber@trash.net> writes:
>> 
>> >> +struct snet_event {
>> >> +	enum snet_syscall syscall;
>> >> +	u8 protocol;
>> >> +} __attribute__ ((packed));
>> >
>> > Does this really need to be packed? You're using it in a
>> > struct snet_event_entry, which is padded anyways.
>> 
>> I think that that members needs to be aligned, because this struct is
>> used for the jhash() computation.
>> when testing on x86_64, I discovered that jhash value wasn't the same,
>> for a same struct snet_event. Then I thougth about misaligned data, and
>> possible 'hole' between syscall and protocol.
>
> Without padding it will eat additional bytes after 'protocol' field,
> since enum is 32-bit long.

thanks for this help.


>> anyway, I patched the code to use jhash_2words() or jhash_1word().
>> here is a patch, which apply on top of initial serie
>
> What's the purpose of hashing verdict_id?

indeed, there is no specific purpose, I will fix this now.

thanks, and again sorry for the delay.
sam
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Evgeniy Polyakov - Jan. 23, 2010, 2:18 a.m.
On Sat, Jan 23, 2010 at 03:07:36AM +0100, Samir Bellabes (sam@synack.fr) wrote:
> I'm sorry for the delay I missed this one.

:)

Patch

diff --git a/security/snet/include/snet.h b/security/snet/include/snet.h
index 47da614..75b32d5 100644
--- a/security/snet/include/snet.h
+++ b/security/snet/include/snet.h
@@ -9,6 +9,6 @@ 
 struct snet_event {
 	enum snet_syscall syscall;
 	u8 protocol;
-} __attribute__ ((packed));
+};
 
 #endif /* _SNET_H */
diff --git a/security/snet/snet_event.c b/security/snet/snet_event.c
index 9f0b0f3..cc3b6a2 100644
--- a/security/snet/snet_event.c
+++ b/security/snet/snet_event.c
@@ -24,17 +24,12 @@  static struct snet_event_entry *__snet_event_lookup(const enum snet_syscall sysc
 	unsigned int h = 0;
 	struct list_head *l;
 	struct snet_event_entry *s;
-	struct snet_event t;
 
 	if (!snet_evh)
 		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) % snet_evh_size;
+	h = jhash_2words(syscall, protocol, 0) % snet_evh_size;
 	l = &snet_evh[h];
 
 	list_for_each_entry(s, l, list) {
@@ -127,7 +122,7 @@  int snet_event_insert(const enum snet_syscall syscall, const u8 protocol)
 	data->se.syscall = syscall;
 	data->se.protocol = protocol;
 	INIT_LIST_HEAD(&(data->list));
-	h = jhash(&(data->se), sizeof(struct snet_event), 0) % snet_evh_size;
+	h = jhash_2words(data->se.syscall, data->se.protocol, 0) % snet_evh_size;
 	list_add_tail(&data->list, &snet_evh[h]);
 	write_unlock_bh(&snet_evh_lock);
 	pr_debug("[%u]=(syscall=%s, protocol=%u)\n",
diff --git a/security/snet/snet_verdict.c b/security/snet/snet_verdict.c
index 0cad06b..477af3b 100644
--- a/security/snet/snet_verdict.c
+++ b/security/snet/snet_verdict.c
@@ -29,18 +29,16 @@  static struct snet_verdict_entry *__snet_verdict_lookup(const u32 verdict_id)
 	unsigned int h = 0;
 	struct list_head *l = NULL;
 	struct snet_verdict_entry *s = NULL;
-	u32 vid = 0;
 
 	if (!snet_vdh)
 		return NULL;
 
-	vid = verdict_id;
 	/* computing its hash value */
-	h = jhash(&vid, sizeof(u32), 0) % snet_vdh_size;
+	h = jhash_1word(verdict_id, 0) % snet_vdh_size;
 	l = &snet_vdh[h];
 
 	list_for_each_entry(s, l, list) {
-		if (s->verdict_id == vid) {
+		if (s->verdict_id == verdict_id) {
 			return s;
 		}
 	}
@@ -134,7 +132,7 @@  int snet_verdict_insert(void)
 	data->verdict_id = verdict_id;
 	data->verdict = SNET_VERDICT_PENDING;
 	INIT_LIST_HEAD(&(data->list));
-	h = jhash(&(data->verdict_id), sizeof(u32), 0) % snet_vdh_size;
+	h = jhash_1word(data->verdict_id, 0) % snet_vdh_size;
 
 	write_lock_bh(&snet_vdh_lock);
 	if (snet_vdh) {