Re: [PATCH] enhance usability of /proc/sys/net/ipv4/ip_local_reserved_ports (v2)

[Stephen Hemminger <shemminger@vyatta.com>]

On Wed, 14 Mar 2012 23:06:33 +0100
Helge Deller <deller@gmx.de> wrote:

> On 03/14/2012 08:43 AM, Cong Wang wrote:
> > On Tue, 2012-03-13 at 21:33 +0100, Helge Deller wrote:
> >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> >> index f487f25..1f60398 100644
> >> --- a/kernel/sysctl.c
> >> +++ b/kernel/sysctl.c
> >> @@ -2805,6 +2805,8 @@ static int proc_do_cad_pid(struct ctl_table *table, int write,
> >>   * We use a range comma separated format (e.g. 1,3-4,10-10) so that
> >>   * large bitmaps may be represented in a compact manner. Writing into
> >>   * the file will clear the bitmap then update it with the given input.
> >> + * If "add" or "release" is written in front of numbers or number ranges,
> >> + * the given bits will be added to or released from the existing bitmap.
> >>   *
> > 
> > What if I only write "add" or "release" ("add ", "release " too) into
> > this file? Make sure you have tested this corner case.
> 
> Sure, I tested this case. It will not modify the the current port list.
> 
> But there were other cases which I initially didn't took care of, mostly 
> because I wanted to keep the parser simple.
> Now, in the next version of the patch the following cases will be handled
> correctly: 
> - "add release 100"		(->syntax error)
> - "release 100 add 100"		(->with all in one line the result will not be
> 		as expected because first bitmap_or() and then bitmap_andnot()
> 		will be executed, so that the 100th bit becomes released instead
> 		of added. Users will need to split this into two echo commands
> 		otherwise -EINVAL will be returned.)
> 
> >>   * Returns 0 on success.
> >>   */
> >> @@ -2813,11 +2815,13 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
> >>  {
> >>  	int err = 0;
> >>  	bool first = 1;
> >> +	bool add_or_release = 0, xrelease = 0;
> >>  	size_t left = *lenp;
> >>  	unsigned long bitmap_len = table->maxlen;
> >>  	unsigned long *bitmap = (unsigned long *) table->data;
> >> -	unsigned long *tmp_bitmap = NULL;
> >> -	char tr_a[] = { '-', ',', '\n' }, tr_b[] = { ',', '\n', 0 }, c;
> >> +	unsigned long *tmp_bitmap = NULL, *release_bitmap = NULL;
> >> +	char tr_a[] = { '-', ',', ' ', '\n' },
> >> +	     tr_b[] = { ',', ' ', '\n', 0 }, c;
> >>  
> >>  	if (!bitmap_len || !left || (*ppos && !write)) {
> >>  		*lenp = 0;
> >> @@ -2841,8 +2845,9 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
> >>                  }
> >>  		kbuf[left] = 0;
> >>  
> >> -		tmp_bitmap = kzalloc(BITS_TO_LONGS(bitmap_len) * sizeof(unsigned long),
> >> -				     GFP_KERNEL);
> >> +		tmp_bitmap = kzalloc(2 * BITS_TO_LONGS(bitmap_len) *
> >> +					sizeof(unsigned long), GFP_KERNEL);
> >> +		release_bitmap = &tmp_bitmap[BITS_TO_LONGS(bitmap_len)];
> > 
> > So you double the size, and give the second half to 'release_bitmap',
> > this will waste spaces when release_bitmap is short, right?
> 
> Yes.
> 
> > *I think* we
> > can check if we want to release any bitmaps first, and then only
> > allocate one of tmp_bitmap and release_bitmap.
> 
> The simpliest solution would be to use strcasestr(kbuf,"release") but
> this function isn't available in the kernel.
> Alternatively I could just search for e.g. upper- and lowercase 
> "release", but I don't like that either. 
> Maybe you have a better idea?
> 
> Overall, 65536 bits (ports) for the ip_local_reserved_ports bitfield
> occupies 8K. With my patch this now becomes 16K. For desktop/server
> usages I think this is OK, esp. since it's only used temporarily and
> freed directly after usage again.
> 
> >>  		if (!tmp_bitmap) {
> >>  			free_page(page);
> >>  			return -ENOMEM;
> >> @@ -2850,7 +2855,32 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
> >>  		proc_skip_char(&kbuf, &left, '\n');
> >>  		while (!err && left) {
> >>  			unsigned long val_a, val_b;
> >> -			bool neg;
> >> +			bool neg, found;
> >> +
> >> +			left -= proc_skip_spaces(&kbuf);
> >> +			if (!left)
> >> +				continue;
> >> +
> >> +			if (first || add_or_release) {
> >> +				found = (0 == strnicmp(kbuf, "add ", 4));
> >> +				if (found) {
> > 
> > I think we don't need an extra variable 'found' here.
> 

This is getting to be a text book example of why /proc is ugly
as a general purpose API.