[PATCH net] ppp: lock ppp->flags in ppp_read() and ppp_poll()

[PATCH net] ppp: lock ppp->flags in ppp_read() and ppp_poll()

[Guillaume Nault]

ppp_read() and ppp_poll() can be called concurrently with ppp_ioctl().
In this case, ppp_ioctl() might call ppp_ccp_closed(), which may update
ppp->flags while ppp_read() or ppp_poll() is reading it.
The update done by ppp_ccp_closed() isn't atomic due to the bit mask
operation ('ppp->flags &= ~(SC_CCP_OPEN | SC_CCP_UP)'), so concurrent
readers might get transient values.
Reading incorrect ppp->flags may disturb the 'ppp->flags & SC_LOOP_TRAFFIC'
test in ppp_read() and ppp_poll(), which in turn can lead to improper
decision on whether the PPP unit file is ready for reading or not.

Since ppp_ccp_closed() is protected by the Rx and Tx locks (with
ppp_lock()), taking the Rx lock is enough for ppp_read() and ppp_poll()
to guarantee that ppp_ccp_closed() won't update ppp->flags
concurrently.

The same reasoning applies to ppp->n_channels. The 'n_channels' field
can also be written to concurrently by ppp_ioctl() (through
ppp_connect_channel() or ppp_disconnect_channel()). These writes aren't
atomic (simple increment/decrement), but are protected by both the Rx
and Tx locks (like in the ppp->flags case). So holding the Rx lock
before reading ppp->n_channels also prevents concurrent writes.

Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---

This was patch #2 of the 'ppp: fix locking issues related to ppp_ioctl()'
series. I haven't kept the extra locking of ppp->flags in
ppp_ioctl(PPPIOCGFLAGS), which was added in the original series,
because the ppp_mutex lock ensures we can't enter the PPPIOCSFLAGS case
concurrently.
This is still quite theoretical issue as I've never observed the error
in practice.

 drivers/net/ppp/ppp_generic.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index fc8ad00..e8a5936 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -443,9 +443,14 @@ static ssize_t ppp_read(struct file *file, char __user *buf,
 			 * network traffic (demand mode).
 			 */
 			struct ppp *ppp = PF_TO_PPP(pf);
+
+			ppp_recv_lock(ppp);
 			if (ppp->n_channels == 0 &&
-			    (ppp->flags & SC_LOOP_TRAFFIC) == 0)
+			    (ppp->flags & SC_LOOP_TRAFFIC) == 0) {
+				ppp_recv_unlock(ppp);
 				break;
+			}
+			ppp_recv_unlock(ppp);
 		}
 		ret = -EAGAIN;
 		if (file->f_flags & O_NONBLOCK)
@@ -532,9 +537,12 @@ static unsigned int ppp_poll(struct file *file, poll_table *wait)
 	else if (pf->kind == INTERFACE) {
 		/* see comment in ppp_read */
 		struct ppp *ppp = PF_TO_PPP(pf);
+
+		ppp_recv_lock(ppp);
 		if (ppp->n_channels == 0 &&
 		    (ppp->flags & SC_LOOP_TRAFFIC) == 0)
 			mask |= POLLIN | POLLRDNORM;
+		ppp_recv_unlock(ppp);
 	}
 
 	return mask;
-- 
2.7.0

RE: [PATCH net] ppp: lock ppp->flags in ppp_read() and ppp_poll()

[David Laight]

From: Guillaume Nault
> Sent: 26 February 2016 17:46
> 
> ppp_read() and ppp_poll() can be called concurrently with ppp_ioctl().
> In this case, ppp_ioctl() might call ppp_ccp_closed(), which may update
> ppp->flags while ppp_read() or ppp_poll() is reading it.
> The update done by ppp_ccp_closed() isn't atomic due to the bit mask
> operation ('ppp->flags &= ~(SC_CCP_OPEN | SC_CCP_UP)'), so concurrent
> readers might get transient values.
> Reading incorrect ppp->flags may disturb the 'ppp->flags & SC_LOOP_TRAFFIC'
> test in ppp_read() and ppp_poll(), which in turn can lead to improper
> decision on whether the PPP unit file is ready for reading or not.
> 
> Since ppp_ccp_closed() is protected by the Rx and Tx locks (with
> ppp_lock()), taking the Rx lock is enough for ppp_read() and ppp_poll()
> to guarantee that ppp_ccp_closed() won't update ppp->flags
> concurrently.

This is all splurious.
The 'concurrent' calls cannot be distinguished from calls just prior to,
or just after the ppp_ccp_closed() call.

So the additional locking (which is covering a single memory read)
cannot be needed.
Only code that modifies the flags needs to be locked.

	David

> The same reasoning applies to ppp->n_channels. The 'n_channels' field
> can also be written to concurrently by ppp_ioctl() (through
> ppp_connect_channel() or ppp_disconnect_channel()). These writes aren't
> atomic (simple increment/decrement), but are protected by both the Rx
> and Tx locks (like in the ppp->flags case). So holding the Rx lock
> before reading ppp->n_channels also prevents concurrent writes.
> 
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
> ---
> 
> This was patch #2 of the 'ppp: fix locking issues related to ppp_ioctl()'
> series. I haven't kept the extra locking of ppp->flags in
> ppp_ioctl(PPPIOCGFLAGS), which was added in the original series,
> because the ppp_mutex lock ensures we can't enter the PPPIOCSFLAGS case
> concurrently.
> This is still quite theoretical issue as I've never observed the error
> in practice.
> 
>  drivers/net/ppp/ppp_generic.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index fc8ad00..e8a5936 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -443,9 +443,14 @@ static ssize_t ppp_read(struct file *file, char __user *buf,
>  			 * network traffic (demand mode).
>  			 */
>  			struct ppp *ppp = PF_TO_PPP(pf);
> +
> +			ppp_recv_lock(ppp);
>  			if (ppp->n_channels == 0 &&
> -			    (ppp->flags & SC_LOOP_TRAFFIC) == 0)
> +			    (ppp->flags & SC_LOOP_TRAFFIC) == 0) {
> +				ppp_recv_unlock(ppp);
>  				break;
> +			}
> +			ppp_recv_unlock(ppp);
>  		}
>  		ret = -EAGAIN;
>  		if (file->f_flags & O_NONBLOCK)
> @@ -532,9 +537,12 @@ static unsigned int ppp_poll(struct file *file, poll_table *wait)
>  	else if (pf->kind == INTERFACE) {
>  		/* see comment in ppp_read */
>  		struct ppp *ppp = PF_TO_PPP(pf);
> +
> +		ppp_recv_lock(ppp);
>  		if (ppp->n_channels == 0 &&
>  		    (ppp->flags & SC_LOOP_TRAFFIC) == 0)
>  			mask |= POLLIN | POLLRDNORM;
> +		ppp_recv_unlock(ppp);
>  	}
> 
>  	return mask;
> --
> 2.7.0

Re: [PATCH net] ppp: lock ppp->flags in ppp_read() and ppp_poll()

[Guillaume Nault]

On Mon, Feb 29, 2016 at 10:18:38AM +0000, David Laight wrote:
> From: Guillaume Nault
> > Sent: 26 February 2016 17:46
> > 
> > ppp_read() and ppp_poll() can be called concurrently with ppp_ioctl().
> > In this case, ppp_ioctl() might call ppp_ccp_closed(), which may update
> > ppp->flags while ppp_read() or ppp_poll() is reading it.
> > The update done by ppp_ccp_closed() isn't atomic due to the bit mask
> > operation ('ppp->flags &= ~(SC_CCP_OPEN | SC_CCP_UP)'), so concurrent
> > readers might get transient values.
> > Reading incorrect ppp->flags may disturb the 'ppp->flags & SC_LOOP_TRAFFIC'
> > test in ppp_read() and ppp_poll(), which in turn can lead to improper
> > decision on whether the PPP unit file is ready for reading or not.
> > 
> > Since ppp_ccp_closed() is protected by the Rx and Tx locks (with
> > ppp_lock()), taking the Rx lock is enough for ppp_read() and ppp_poll()
> > to guarantee that ppp_ccp_closed() won't update ppp->flags
> > concurrently.
> 
> This is all splurious.
> The 'concurrent' calls cannot be distinguished from calls just prior to,
> or just after the ppp_ccp_closed() call.
> 
If the ppp->flags update is guaranteed to be atomic from a reader's
point of view, then yes, concurrent runs of ppp_{read,poll}() and
ppp_ccp_closed() aren't an issue.

Here I assume that 'ppp->flags &= ~(SC_CCP_OPEN | SC_CCP_UP)' isn't
atomic, and that a concurrent reader may get a value different from
the original or final value of ppp->flags. If ppp_read() or ppp_poll()
reads such a transient value that affects the SC_LOOP_TRAFFIC bit, the
code may take the wrong decision as to whether or not the file
descriptor is available for reading.

ppp_ioctl()
   |
   \_ ppp_ccp_closed()
            |
	    \_ (1) Start Read-Modify-Write operation on ppp->flags
	       |
	       |
	       | <-- (2) ppp_read() or ppp_poll() reads transient value
	       |         of ppp->flags here
	       |
	       (3) End Read-Modify-Write operation on ppp->flags

If we assume that the RMW operation can't generate transient values, or
that transient values can't affect the SC_LOOP_TRAFFIC bit, then this
patch is not needed. However I'm not aware of such guarantees (unless
the atomic stores of ints described by DaveM in my original series
also applies here). At least I haven't seen other parts of the stack
implicitely relying in atomic properties of RMW operations performed on
plain integers.

If my analysis is too far fetched, I'll happily drop the patch.
I'm just trying to fix the possible issues I saw while working on the
PPP code, before resubmitting the PPP rtnetlink handler support.

Re: [PATCH net] ppp: lock ppp->flags in ppp_read() and ppp_poll()

[David Miller]

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Fri, 26 Feb 2016 18:45:34 +0100

> @@ -443,9 +443,14 @@ static ssize_t ppp_read(struct file *file, char __user *buf,
>  			 * network traffic (demand mode).
>  			 */
>  			struct ppp *ppp = PF_TO_PPP(pf);
> +
> +			ppp_recv_lock(ppp);
>  			if (ppp->n_channels == 0 &&
> -			    (ppp->flags & SC_LOOP_TRAFFIC) == 0)
> +			    (ppp->flags & SC_LOOP_TRAFFIC) == 0) {
> +				ppp_recv_unlock(ppp);
>  				break;
> +			}

I agree with this change because these two tests much be done without either
of them changing meanwhile, so I'll apply this to 'net', thanks!