drivers/net/ppp/ppp_generic.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
ppp_fill_forward_path() has two race conditions:
1. The ppp->channels list can change between list_empty() and
list_first_entry(), as ppp_lock() is not held. If the only channel
is deleted in ppp_disconnect_channel(), list_first_entry() may
access an empty head or a freed entry, and trigger a panic.
2. pch->chan can be NULL. When ppp_unregister_channel() is called,
pch->chan is set to NULL before pch is removed from ppp->channels.
Fix these by using a lockless RCU approach:
- Use list_first_or_null_rcu() to safely test and access the first list
entry.
- Convert list modifications on ppp->channels to their RCU variants and
add synchronize_rcu() after removal.
- Check for a NULL pch->chan before dereferencing it.
Fixes: f6efc675c9dd ("net: ppp: resolve forwarding path for bridge pppoe devices")
Signed-off-by: Qingfang Deng <dqfext@gmail.com>
---
drivers/net/ppp/ppp_generic.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 8c98cbd4b06d..fd3ac75a56e3 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -33,6 +33,7 @@
#include <linux/ppp_channel.h>
#include <linux/ppp-comp.h>
#include <linux/skbuff.h>
+#include <linux/rculist.h>
#include <linux/rtnetlink.h>
#include <linux/if_arp.h>
#include <linux/ip.h>
@@ -1598,11 +1599,14 @@ static int ppp_fill_forward_path(struct net_device_path_ctx *ctx,
if (ppp->flags & SC_MULTILINK)
return -EOPNOTSUPP;
- if (list_empty(&ppp->channels))
+ pch = list_first_or_null_rcu(&ppp->channels, struct channel, clist);
+ if (!pch)
return -ENODEV;
- pch = list_first_entry(&ppp->channels, struct channel, clist);
chan = pch->chan;
+ if (!chan)
+ return -ENODEV;
+
if (!chan->ops->fill_forward_path)
return -EOPNOTSUPP;
@@ -3515,7 +3519,7 @@ ppp_connect_channel(struct channel *pch, int unit)
hdrlen = pch->file.hdrlen + 2; /* for protocol bytes */
if (hdrlen > ppp->dev->hard_header_len)
ppp->dev->hard_header_len = hdrlen;
- list_add_tail(&pch->clist, &ppp->channels);
+ list_add_tail_rcu(&pch->clist, &ppp->channels);
++ppp->n_channels;
pch->ppp = ppp;
refcount_inc(&ppp->file.refcnt);
@@ -3545,10 +3549,11 @@ ppp_disconnect_channel(struct channel *pch)
if (ppp) {
/* remove it from the ppp unit's list */
ppp_lock(ppp);
- list_del(&pch->clist);
+ list_del_rcu(&pch->clist);
if (--ppp->n_channels == 0)
wake_up_interruptible(&ppp->file.rwait);
ppp_unlock(ppp);
+ synchronize_rcu();
if (refcount_dec_and_test(&ppp->file.refcnt))
ppp_destroy_interface(ppp);
err = 0;
--
2.43.0On Mon, Aug 11, 2025 at 1:44 AM Qingfang Deng <dqfext@gmail.com> wrote:
>
> ppp_fill_forward_path() has two race conditions:
>
> 1. The ppp->channels list can change between list_empty() and
> list_first_entry(), as ppp_lock() is not held. If the only channel
> is deleted in ppp_disconnect_channel(), list_first_entry() may
> access an empty head or a freed entry, and trigger a panic.
>
> 2. pch->chan can be NULL. When ppp_unregister_channel() is called,
> pch->chan is set to NULL before pch is removed from ppp->channels.
>
> Fix these by using a lockless RCU approach:
> - Use list_first_or_null_rcu() to safely test and access the first list
> entry.
> - Convert list modifications on ppp->channels to their RCU variants and
> add synchronize_rcu() after removal.
> - Check for a NULL pch->chan before dereferencing it.
>
> Fixes: f6efc675c9dd ("net: ppp: resolve forwarding path for bridge pppoe devices")
> Signed-off-by: Qingfang Deng <dqfext@gmail.com>
> ---
> drivers/net/ppp/ppp_generic.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 8c98cbd4b06d..fd3ac75a56e3 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -33,6 +33,7 @@
> #include <linux/ppp_channel.h>
> #include <linux/ppp-comp.h>
> #include <linux/skbuff.h>
> +#include <linux/rculist.h>
> #include <linux/rtnetlink.h>
> #include <linux/if_arp.h>
> #include <linux/ip.h>
> @@ -1598,11 +1599,14 @@ static int ppp_fill_forward_path(struct net_device_path_ctx *ctx,
> if (ppp->flags & SC_MULTILINK)
> return -EOPNOTSUPP;
>
> - if (list_empty(&ppp->channels))
> + pch = list_first_or_null_rcu(&ppp->channels, struct channel, clist);
It is unclear if rcu_read_lock() is held at this point.
list_first_or_null_rcu() does not have a builtin __list_check_rcu()
> + if (!pch)
> return -ENODEV;
>
> - pch = list_first_entry(&ppp->channels, struct channel, clist);
> chan = pch->chan;
chan = READ_ONCE(pch->chan);
And add a WRITE_ONCE(pch->chan, NULL) in ppp_unregister_channel()
And/or add __rcu to pch->chan
> + if (!chan)
> + return -ENODEV;
> +
> if (!chan->ops->fill_forward_path)
> return -EOPNOTSUPP;
>
> @@ -3515,7 +3519,7 @@ ppp_connect_channel(struct channel *pch, int unit)
> hdrlen = pch->file.hdrlen + 2; /* for protocol bytes */
> if (hdrlen > ppp->dev->hard_header_len)
> ppp->dev->hard_header_len = hdrlen;
> - list_add_tail(&pch->clist, &ppp->channels);
> + list_add_tail_rcu(&pch->clist, &ppp->channels);
> ++ppp->n_channels;
> pch->ppp = ppp;
> refcount_inc(&ppp->file.refcnt);
> @@ -3545,10 +3549,11 @@ ppp_disconnect_channel(struct channel *pch)
> if (ppp) {
> /* remove it from the ppp unit's list */
> ppp_lock(ppp);
> - list_del(&pch->clist);
> + list_del_rcu(&pch->clist);
> if (--ppp->n_channels == 0)
> wake_up_interruptible(&ppp->file.rwait);
> ppp_unlock(ppp);
> + synchronize_rcu();
synchronize_net() is preferred.
> if (refcount_dec_and_test(&ppp->file.refcnt))
> ppp_destroy_interface(ppp);
> err = 0;
> --
> 2.43.0
>
On Mon, Aug 11, 2025 at 5:19 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Aug 11, 2025 at 1:44 AM Qingfang Deng <dqfext@gmail.com> wrote:
> It is unclear if rcu_read_lock() is held at this point.
>
> list_first_or_null_rcu() does not have a builtin __list_check_rcu()
ndo_fill_forward_path() is called by nf_tables chains, which is inside
an RCU critical section.
> > chan = pch->chan;
>
> chan = READ_ONCE(pch->chan);
>
> And add a WRITE_ONCE(pch->chan, NULL) in ppp_unregister_channel()
>
> And/or add __rcu to pch->chan
Should I add {READ,WRITE}_ONCE to all occurrences of pch->chan or only
to ppp_unregister_channel?
>
> > + synchronize_rcu();
>
> synchronize_net() is preferred.
>
Noted.
Thanks!
On Mon, Aug 11, 2025 at 5:35 PM Qingfang Deng <dqfext@gmail.com> wrote: > > On Mon, Aug 11, 2025 at 5:19 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Mon, Aug 11, 2025 at 1:44 AM Qingfang Deng <dqfext@gmail.com> wrote: > > It is unclear if rcu_read_lock() is held at this point. > > > > list_first_or_null_rcu() does not have a builtin __list_check_rcu() > > ndo_fill_forward_path() is called by nf_tables chains, which is inside > an RCU critical section. Update: mtk_flow_get_wdma_info() in mtk_ppe_offload.c calls dev_fill_forward_path() in process context without RCU, so ppp_fill_forward_path() can be called from two different contexts. Should I add rcu_read_lock() to mtk_flow_get_wdma_info() or ppp_fill_forward_path()?
On Tue, Aug 12, 2025 at 05:38:02PM +0800, Qingfang Deng wrote: > On Mon, Aug 11, 2025 at 5:35 PM Qingfang Deng <dqfext@gmail.com> wrote: > > > > On Mon, Aug 11, 2025 at 5:19 PM Eric Dumazet <edumazet@google.com> wrote: > > > > > > On Mon, Aug 11, 2025 at 1:44 AM Qingfang Deng <dqfext@gmail.com> wrote: > > > It is unclear if rcu_read_lock() is held at this point. > > > > > > list_first_or_null_rcu() does not have a builtin __list_check_rcu() > > > > ndo_fill_forward_path() is called by nf_tables chains, which is inside > > an RCU critical section. > > Update: mtk_flow_get_wdma_info() in mtk_ppe_offload.c calls > dev_fill_forward_path() in process context without RCU, so > ppp_fill_forward_path() can be called from two different contexts. > Should I add rcu_read_lock() to mtk_flow_get_wdma_info() or > ppp_fill_forward_path()? mtk_flow_get_wdma_info() seems to be the exception at this point, so I'm inclined to add rcu_read_lock() to mtk_flow_get_wdma_info().
On Tue, Aug 12, 2025 at 9:02 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > mtk_flow_get_wdma_info() seems to be the exception at this point, so > I'm inclined to add rcu_read_lock() to mtk_flow_get_wdma_info(). Okay. I'll send a v2 with this change.
© 2016 - 2025 Red Hat, Inc.