svn commit: r346319 - head/sys/netpfil/pf

[Kristof Provost]

On 17 Apr 2019, at 22:17, Gleb Smirnoff wrote:
>   Kristof,
>
> On Wed, Apr 17, 2019 at 04:42:54PM +0000, Kristof Provost wrote:
> K> Modified: head/sys/netpfil/pf/pf_ioctl.c
> K> 
> ==============================================================================
> K> --- head/sys/netpfil/pf/pf_ioctl.c	Wed Apr 17 16:31:30 
> 2019	(r346318)
> K> +++ head/sys/netpfil/pf/pf_ioctl.c	Wed Apr 17 16:42:54 
> 2019	(r346319)
> K> @@ -3103,24 +3103,24 @@ DIOCCHANGEADDR_error:
> K>  			break;
> K>  		}
> K>
> K> -		PF_RULES_WLOCK();
> K> +		PF_RULES_RLOCK();
> K>  		n = pfr_table_count(&io->pfrio_table, io->pfrio_flags);
> K>  		io->pfrio_size = min(io->pfrio_size, n);
> K> +		PF_RULES_RUNLOCK();
> K>
> K>  		totlen = io->pfrio_size * sizeof(struct pfr_table);
> K>  		pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
> K>  		    M_TEMP, M_NOWAIT);
> K>  		if (pfrts == NULL) {
> K>  			error = ENOMEM;
> K> -			PF_RULES_WUNLOCK();
> K>  			break;
> K>  		}
> K>  		error = copyin(io->pfrio_buffer, pfrts, totlen);
> K>  		if (error) {
> K>  			free(pfrts, M_TEMP);
> K> -			PF_RULES_WUNLOCK();
> K>  			break;
> K>  		}
> K> +		PF_RULES_WLOCK();
> K>  		error = pfr_set_tflags(pfrts, io->pfrio_size,
> K>  		    io->pfrio_setflag, io->pfrio_clrflag, &io->pfrio_nchange,
> K>  		    &io->pfrio_ndel, io->pfrio_flags | PFR_FLAG_USERIOCTL);
>
> Couple comments:
>
> 1) Now we can malloc with M_WAITOK.
>
That’s a good point. I’ll see about changing that tomorrow.

> 2) Are we sure that table count won't change while we dropped the 
> lock?
>
No, the table count can indeed change while we’re unlocked. It 
doesn’t really matter though. The initial count only serves to limit 
the memory allocation to something sane.  pfr_set_tflags() still does 
appropriate checks.
It’s always been possible for the table count to change between user 
space preparing its request and it being handled in the kernel, so that 
was always a possibility.

Regards,
Kristof