Re: [PATCH] SUNRPC: cache: ignore timestamp written to 'flush' file.

["J. Bruce Fields" <bfields@fieldses.org>]

On Wed, Feb 14, 2018 at 12:15:06PM +1100, NeilBrown wrote:
> 
> The interface for flushing the sunrpc auth cache was poorly
> designed and has caused problems a number of times.
> 
> The design is that you write a timestamp, and all entries
> created before that time are discarded.
> The most obvious problem is that this is not what people
> actually want.  They want to just flush the whole cache.
> The 1-second granularity can be a problem, as can the use
> of wall-clock time.
> 
> A current problem is that code will write the current time to
> this file - expecting it to clear everything - and if the
> seconds number ticks over before this timestamp is checked,
> the test "then >= now" fails, and a full flush isn't forced.
> 
> So lets just drop the subtleties and always flush the whole
> cache.  The worst this could do is impose an extra cost
> refilling it, but that would require someone to be using
> non-standard tools.
> 
> We still report an error if the string written is not a number,
> but we cause any valid number to flush the whole cache.

Thanks, applying for 4.17 (unless someone thinks it's more urgent).

--b.

> 
> Reported-by: "Wang, Alan 1. (NSB - CN/Hangzhou)" <alan.1.wang@nokia-sbell.com>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  net/sunrpc/cache.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index 8a7e1c774f9c..26582e27be6a 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -1450,8 +1450,8 @@ static ssize_t write_flush(struct file *file, const char __user *buf,
>  			   struct cache_detail *cd)
>  {
>  	char tbuf[20];
> -	char *bp, *ep;
> -	time_t then, now;
> +	char *ep;
> +	time_t now;
>  
>  	if (*ppos || count > sizeof(tbuf)-1)
>  		return -EINVAL;
> @@ -1461,24 +1461,24 @@ static ssize_t write_flush(struct file *file, const char __user *buf,
>  	simple_strtoul(tbuf, &ep, 0);
>  	if (*ep && *ep != '\n')
>  		return -EINVAL;
> +	/* Note that while we check that 'buf' holds a valid number,
> +	 * we always ignore the value and just flush everything.
> +	 * Making use of the number leads to races.
> +	 */
>  
> -	bp = tbuf;
> -	then = get_expiry(&bp);
>  	now = seconds_since_boot();
> -	cd->nextcheck = now;
> -	/* Can only set flush_time to 1 second beyond "now", or
> -	 * possibly 1 second beyond flushtime.  This is because
> -	 * flush_time never goes backwards so it mustn't get too far
> -	 * ahead of time.
> +	/* Always flush everything, so behave like cache_purge()
> +	 * Do this by advancing flush_time to the current time,
> +	 * or by one second if it has already reached the current time.
> +	 * Newly added cache entries will always have ->last_refresh greater
> +	 * that ->flush_time, so they don't get flushed prematurely.
>  	 */
> -	if (then >= now) {
> -		/* Want to flush everything, so behave like cache_purge() */
> -		if (cd->flush_time >= now)
> -			now = cd->flush_time + 1;
> -		then = now;
> -	}
>  
> -	cd->flush_time = then;
> +	if (cd->flush_time >= now)
> +		now = cd->flush_time + 1;
> +
> +	cd->flush_time = now;
> +	cd->nextcheck = now;
>  	cache_flush();
>  
>  	*ppos += count;
> -- 
> 2.14.0.rc0.dirty
>