sysctl: Fix proc_do_large_bitmap for large input buffers

Today, proc_do_large_bitmap() truncates a large write input buffer
to PAGE_SIZE - 1, which may result in misparsed numbers at the
(truncated) end of the buffer.  Further, it fails to notify the caller
that the buffer was truncated, so it doesn't get called iteratively
to finish the entire input buffer.

Tell the caller if there's more work to do by adding the skipped
amount back to left/*lenp before returning.

To fix the misparsing, reset the position if we have completely
consumed a truncated buffer (or if just one char is left, which
may be a "-" in a range), and ask the caller to come back for
more.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Here's a pretty hacky test script to test this code via
ip_local_reserved_ports

-----

#!/bin/bash

# Randomly construct well-formed (sequential, non-overlapping)
# input for ip_local_reserved_ports, feed it to the sysctl,
# then read it back and check for differences.

# Port range to use
PORT_START=1024
PORT_STOP=32768

# Total length of ports string to use
LENGTH=$((4096+$((RANDOM % 16384))))

# String containing our list of ports
PORTS=$PORT_START

# Try 1000 times
for I in `seq 1 1000`; do
	
	# build up the string
	while true; do
		# Make sure it's discontiguous, skip ahead at least 2
		SKIP=$((2 + RANDOM % 10))
		PORT_START=$((PORT_START + SKIP))
	
		if [ "$PORT_START" -ge "$PORT_STOP" ]; then
			break;
		fi
	
		# 14856-14863,14861
		# Add a range, or a single port
		USERANGE=$((RANDOM % 2))
	
		if [ "$USERANGE" -eq "1" ]; then
			RANGE_START=$PORT_START
			RANGE_LEN=$((1 + RANDOM % 10))
			RANGE_END=$((RANGE_START + RANGE_LEN))
			PORTS="${PORTS},${RANGE_START}-${RANGE_END}"
			# Break out if we've done enough
			if [ "$RANGE_END" -eq "$PORT_STOP" ]; then
				break;
			fi
			PORT_START=$RANGE_END
		else
			PORTS="${PORTS},${PORT_START}"
		fi
	
		if [ "${#PORTS}" -gt "$LENGTH" ]; then
			break;
		fi
	
	done
	
	# See if we get out what we put in
	echo "Trial $I"
	echo $PORTS > port_list
	cat port_list > /proc/sys/net/ipv4/ip_local_reserved_ports || break
	cat /proc/sys/net/ipv4/ip_local_reserved_ports > port_list_out
	diff -uq port_list port_list_out || break
	
done

On Wed, Feb 20, 2019 at 05:35:04PM -0600, Eric Sandeen wrote:
> Here's a pretty hacky test script to test this code via
> ip_local_reserved_ports

Thanks Eric!

So /proc/sys/net/ipv4/ip_local_reserved_ports is a production knob, and
if we wanted to stress test it with a selftest it could break other self
tests or change the system behaviour. Because of this we have now have
lib/test_sysctl.c, and we test this with the script:

tools/testing/selftests/sysctl/sysctl.sh

Any chance you can extend lib/test_sysctl.c with a new respective bitmap
knob, and add a respective test? This will ensure we don't regress
later. 0-day runs sysctl.sh so it should catch any regressions in the
future.

If you can think of other ways to test the knob that would be great too.

  Luis

On 2/21/19 9:18 AM, Luis Chamberlain wrote:
> On Wed, Feb 20, 2019 at 05:35:04PM -0600, Eric Sandeen wrote:
>> Here's a pretty hacky test script to test this code via
>> ip_local_reserved_ports
> 
> Thanks Eric!
> 
> So /proc/sys/net/ipv4/ip_local_reserved_ports is a production knob, and
> if we wanted to stress test it with a selftest it could break other self
> tests or change the system behaviour. Because of this we have now have
> lib/test_sysctl.c, and we test this with the script:
> 
> tools/testing/selftests/sysctl/sysctl.sh
> 
> Any chance you can extend lib/test_sysctl.c with a new respective bitmap
> knob,

Done

> and add a respective test? This will ensure we don't regress
> later. 0-day runs sysctl.sh so it should catch any regressions in the
> future.

As you know, learning somebody else's test harness infra is a PITA. ;)
Can you find me off-list and give me a hand with this?

Thanks,
-Eric

On Thu, Feb 21, 2019 at 11:47:49AM -0600, Eric Sandeen wrote:
> On 2/21/19 9:18 AM, Luis Chamberlain wrote:
> > On Wed, Feb 20, 2019 at 05:35:04PM -0600, Eric Sandeen wrote:
> >> Here's a pretty hacky test script to test this code via
> >> ip_local_reserved_ports
> > 
> > Thanks Eric!
> > 
> > So /proc/sys/net/ipv4/ip_local_reserved_ports is a production knob, and
> > if we wanted to stress test it with a selftest it could break other self
> > tests or change the system behaviour. Because of this we have now have
> > lib/test_sysctl.c, and we test this with the script:
> > 
> > tools/testing/selftests/sysctl/sysctl.sh
> > 
> > Any chance you can extend lib/test_sysctl.c with a new respective bitmap
> > knob,
> 
> Done

Thanks!

> > and add a respective test? This will ensure we don't regress
> > later. 0-day runs sysctl.sh so it should catch any regressions in the
> > future.
> 
> As you know, learning somebody else's test harness infra is a PITA. ;)
> Can you find me off-list and give me a hand with this?

Sure, its actually quite simple, just as root, run the script.

  Luis

On 2/21/19 11:52 AM, Luis Chamberlain wrote:
> On Thu, Feb 21, 2019 at 11:47:49AM -0600, Eric Sandeen wrote:
>> On 2/21/19 9:18 AM, Luis Chamberlain wrote:
>>> On Wed, Feb 20, 2019 at 05:35:04PM -0600, Eric Sandeen wrote:
>>>> Here's a pretty hacky test script to test this code via
>>>> ip_local_reserved_ports
>>>
>>> Thanks Eric!
>>>
>>> So /proc/sys/net/ipv4/ip_local_reserved_ports is a production knob, and
>>> if we wanted to stress test it with a selftest it could break other self
>>> tests or change the system behaviour. Because of this we have now have
>>> lib/test_sysctl.c, and we test this with the script:
>>>
>>> tools/testing/selftests/sysctl/sysctl.sh
>>>
>>> Any chance you can extend lib/test_sysctl.c with a new respective bitmap
>>> knob,
>>
>> Done
> 
> Thanks!
> 
>>> and add a respective test? This will ensure we don't regress
>>> later. 0-day runs sysctl.sh so it should catch any regressions in the
>>> future.
>>
>> As you know, learning somebody else's test harness infra is a PITA. ;)
>> Can you find me off-list and give me a hand with this?
> 
> Sure, its actually quite simple, just as root, run the script.

Running it looks easy.  I'd like to know about how to extend it.

Thanks,
-Eric

On 2/20/19 5:32 PM, Eric Sandeen wrote:
> Today, proc_do_large_bitmap() truncates a large write input buffer
> to PAGE_SIZE - 1, which may result in misparsed numbers at the
> (truncated) end of the buffer.  Further, it fails to notify the caller
> that the buffer was truncated, so it doesn't get called iteratively
> to finish the entire input buffer.
> 
> Tell the caller if there's more work to do by adding the skipped
> amount back to left/*lenp before returning.
> 
> To fix the misparsing, reset the position if we have completely
> consumed a truncated buffer (or if just one char is left, which
> may be a "-" in a range), and ask the caller to come back for
> more.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Would be nice to fix this bug.  I submitted the test node patch as well
as an attempt to integrate it into the test harness, though there's
wonkiness there still, and I could use more experienced eyes.

Can we move this forward?

Thanks,
-Eric

> ---
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index ba4d9e85feb8..970a96659809 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -3089,9 +3089,13 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  
>  	if (write) {
>  		char *kbuf, *p;
> +		size_t skipped = 0;
>  
> -		if (left > PAGE_SIZE - 1)
> +		if (left > PAGE_SIZE - 1) {
>  			left = PAGE_SIZE - 1;
> +			/* How much of the buffer we'll skip this pass */
> +			skipped = *lenp - left;
> +		}
>  
>  		p = kbuf = memdup_user_nul(buffer, left);
>  		if (IS_ERR(kbuf))
> @@ -3108,9 +3112,22 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  		while (!err && left) {
>  			unsigned long val_a, val_b;
>  			bool neg;
> +			size_t saved_left;
>  
> +			/* In case we stop parsing mid-number, we can reset */
> +			saved_left = left;
>  			err = proc_get_long(&p, &left, &val_a, &neg, tr_a,
>  					     sizeof(tr_a), &c);
> +			/*
> +			 * If we consumed the entirety of a truncated buffer or
> +			 * only one char is left (may be a "-"), then stop here,
> +			 * reset, & come back for more.
> +			 */
> +			if ((left <= 1) && skipped) {
> +				left = saved_left;
> +				break;
> +			}
> +
>  			if (err)
>  				break;
>  			if (val_a >= bitmap_len || neg) {
> @@ -3128,6 +3145,15 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  				err = proc_get_long(&p, &left, &val_b,
>  						     &neg, tr_b, sizeof(tr_b),
>  						     &c);
> +				/*
> +				 * If we consumed all of a truncated buffer or
> +				 * then stop here, reset, & come back for more.
> +				 */
> +				if (!left && skipped) {
> +					left = saved_left;
> +					break;
> +				}
> +
>  				if (err)
>  					break;
>  				if (val_b >= bitmap_len || neg ||
> @@ -3146,6 +3172,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  			proc_skip_char(&p, &left, '\n');
>  		}
>  		kfree(kbuf);
> +		left += skipped;
>  	} else {
>  		unsigned long bit_a, bit_b = 0;
>  
> 
>

On Mon, Mar 04, 2019 at 10:43:09PM -0600, Eric Sandeen wrote:
> On 2/20/19 5:32 PM, Eric Sandeen wrote:
> > Today, proc_do_large_bitmap() truncates a large write input buffer
> > to PAGE_SIZE - 1, which may result in misparsed numbers at the
> > (truncated) end of the buffer.  Further, it fails to notify the caller
> > that the buffer was truncated, so it doesn't get called iteratively
> > to finish the entire input buffer.
> > 
> > Tell the caller if there's more work to do by adding the skipped
> > amount back to left/*lenp before returning.
> > 
> > To fix the misparsing, reset the position if we have completely
> > consumed a truncated buffer (or if just one char is left, which
> > may be a "-" in a range), and ask the caller to come back for
> > more.
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Would be nice to fix this bug.  I submitted the test node patch as well
> as an attempt to integrate it into the test harness, though there's
> wonkiness there still, and I could use more experienced eyes.

Sorry for the delay.

I'm rolling these changes in with some minor adjustments, can you send
me your respective lib/test_sysctl.c changes? I don't see that they had
been sent.

  Luis

On Tue, Mar 19, 2019 at 9:30 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> can you send
> me your respective lib/test_sysctl.c changes? I don't see that they had
> been sent.

Never mind, I see it now, will roll all this in.

 Luis