netdev - Re: [PATCH 4/7] Add "depth".

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]

Message-Id: <200707292254.15514.mb@bu3sch.de>
Date:	Sun, 29 Jul 2007 22:54:15 +0200
From:	Michael Buesch <mb@...sch.de>
To:	Corey Hickey <bugfood-ml@...ooh.org>
Cc:	Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: [PATCH 4/7] Add "depth".

On Sunday 29 July 2007 22:21, Corey Hickey wrote:
> > Compare depth against (~0U/2)-1? What's that doing? Should probably add a comment.
> 
> ~0U/2 - 1 is the maximum value depth can be, based on how it is used in 
> indexing q->dep. I agree, though, that deserves a comment. Actually, 
> I'll also change it to '#define SFQ_DEPTH_MAX (~0U/2 - 1)' and put it 
> near the top of the file next to the 'typedef unsigned int sfq_index;'.
> 
> I could also include limits.h and use UINT_MAX instead of ~0U; would 
> that be preferable?

Seems like a good idea.

> >>  
> >>  		if (ctl->limit)
> >> -			q->limit = min_t(u32, ctl->limit, SFQ_DEPTH);
> >> +			q->limit = min_t(u32, ctl->limit, q->depth);
> >>  	}
> >>  
> >> +	q->dep = kmalloc((1+q->depth*2)*sizeof(struct sfq_head), GFP_KERNEL);
> >> +	if (!q->dep)
> >> +		goto err_case;
> >> +	q->next = kmalloc(q->depth*sizeof(sfq_index), GFP_KERNEL);
> >> +	if (!q->next)
> >> +		goto err_case;
> >> +	q->allot = kmalloc(q->depth*sizeof(short), GFP_KERNEL);
> >> +	if (!q->allot)
> >> +		goto err_case;
> >> +	q->hash = kmalloc(q->depth*sizeof(unsigned short), GFP_KERNEL);
> >> +	if (!q->hash)
> >> +		goto err_case;
> >> +	q->qs = kmalloc(q->depth*sizeof(struct sk_buff_head), GFP_KERNEL);
> >> +	if (!q->qs)
> >> +		goto err_case;
> > 
> > You may chose to use kcalloc for array allocations.
> 
> The arrays in the original code don't get zeroed either, so that 
> shouldn't be necessary (and I haven't heard of any problems so far). Do 
> you suggest I use kcalloc() anyway, just as a good practice?

Well, I think we don't have strict rules on that, so it depends
on the developer's taste. The advantage of kcalloc is, that it might
catch errors in the args better than this opencoded multiplication.
(There's some BUG_ON logic in kcalloc)

> >>  	for (i=0; i<SFQ_HASH_DIVISOR; i++)
> >> -		q->ht[i] = SFQ_DEPTH;
> >> -	for (i=0; i<SFQ_DEPTH; i++) {
> >> +		q->ht[i] = q->depth;
> >> +	for (i=0; i<q->depth; i++) {
> >>  		skb_queue_head_init(&q->qs[i]);
> >> -		q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
> >> -		q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
> >> +		q->dep[i+q->depth].next = i+q->depth;
> >> +		q->dep[i+q->depth].prev = i+q->depth;
> >>  	}
> >>  
> >> -	for (i=0; i<SFQ_DEPTH; i++)
> >> +	for (i=0; i<q->depth; i++)
> >>  		sfq_link(q, i);
> >>  	return 0;
> >> +err_case:
> > 
> > This leaks a few kmallocs.
> 
> Are you saying that the 'err_case:' leaks kmallocs? It calls 
> sfq_q_destroy(q), which kfrees each of the arrays: dep, next, allot, 
> hash, and qs. Is that sufficient, or am I missing something or 
> misunderstanding you?

Ok, I didn't see that. So this should be ok.

> >> +	sfq_q_destroy(q);
> >> +	return -ENOBUFS;
> >>  }
-
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