* Re: [PATCH net] atm: pppoatm: reject duplicate backend assignment
2026-09-02 4:03 [PATCH net] atm: pppoatm: reject duplicate backend assignment XingWang Xiang
@ 2026-09-05 19:46 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-05 19:46 UTC (permalink / raw)
To: v3rdant.xiang
Cc: mitch, 3chas3, netdev, davem, edumazet, kuba, pabeni, horms,
linux-atm-general, linux-kernel
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0
- [High] Incomplete fix: the new `if (atmvcc->user_back) return -EBUSY;`
guard in pppoatm_assign_vcc() (net/atm/pppoatm.c:399) only rejects a…
Pre-existing issues:
- [Medium] pppoatm_assign_vcc() publishes the backend with two plain,
unordered stores: `atmvcc->user_back = pvcc;` immediately followed by…
--
atm: pppoatm: reject duplicate backend assignment
This adds a check in pppoatm_assign_vcc() that returns -EBUSY when
atmvcc->user_back is already populated, so a second ATM_SETBACKEND with
ATM_BACKEND_PPP cannot stack a second pppoatm_vcc on the same VCC. The
commit message describes the resulting NULL user_back dereference during
VCC teardown as the bug being fixed.
> diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
> index 6da52d12df68e..93ba22f437bc9 100644
> --- a/net/atm/pppoatm.c
> +++ b/net/atm/pppoatm.c
> @@ -396,6 +396,8 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
> if (be.encaps != PPPOATM_ENCAPS_AUTODETECT &&
> be.encaps != PPPOATM_ENCAPS_VC && be.encaps != PPPOATM_ENCAPS_LLC)
> return -EINVAL;
> + if (atmvcc->user_back)
> + return -EBUSY;
[Severity: High]
Does this guard close the described crash, or only the pppoatm-over-pppoatm
ordering of it? pppoatm is not the only writer of atmvcc->user_back.
br2684_regvcc() in net/atm/br2684.c writes the same field, and its only
ownership checks are:
net/atm/br2684.c:br2684_regvcc() {
...
if (atmvcc->push == NULL) {
err = -EBADFD;
goto error;
}
if (!list_empty(&brdev->brvccs)) {
/* Only 1 VCC/dev right now */
err = -EEXIST;
goto error;
}
...
}
After pppoatm_assign_vcc() has installed pppoatm_push, atmvcc->push is
non-NULL, and a freshly created nas device from ATM_NEWBACKENDIF has an
empty brvccs list, so neither check fires. br2684_regvcc() then does:
net/atm/br2684.c:br2684_regvcc() {
...
atmvcc->user_back = brvcc;
brvcc->encaps = (enum br2684_encaps)be.encaps;
brvcc->old_push = atmvcc->push;
...
}
which overwrites the pppoatm private pointer and chains pppoatm_push as
old_push.
On close, vcc_destroy_socket() calls vcc->push(vcc, NULL) -> br2684_push()
-> br2684_close_vcc():
net/atm/br2684.c:br2684_close_vcc() {
...
brvcc->atmvcc->user_back = NULL; /* what about vcc->recvq ??? */
brvcc->atmvcc->release_cb = brvcc->old_release_cb;
brvcc->old_push(brvcc->atmvcc, NULL); /* pass on the bad news */
...
}
so pppoatm_push() runs with user_back already NULL and dereferences it:
net/atm/pppoatm.c:pppoatm_push() {
...
module = pvcc->old_owner;
pppoatm_unassign_vcc(atmvcc);
...
}
Is that not the same NULL dereference the commit message says is removed,
just with the two ATM_SETBACKEND calls in the opposite order?
While both backends are stacked, can this also corrupt memory? br2684_pop()
calls brvcc->old_pop, which is pppoatm_pop():
net/atm/pppoatm.c:pppoatm_pop() {
struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
pvcc->old_pop(atmvcc, skb);
atomic_dec(&pvcc->inflight);
...
}
Here user_back points at a struct br2684_vcc, so pvcc->old_pop is an
indirect call through br2684_vcc::old_push and atomic_dec(&pvcc->inflight)
writes into br2684_vcc::old_owner. The same type confusion applies to
br2684_release_cb() -> pppoatm_release_cb(). The first pvcc also stays
registered via ppp_register_channel() with a live tasklet, and its
__module_get(THIS_MODULE) is never balanced, since teardown now runs through
br2684_push().
Regarding this part of the commit message:
do_vcc_ioctl() holds ioctl_mutex while invoking backend handlers, so
concurrent backend assignments cannot race past this check.
Is serialization the relevant property here? do_vcc_ioctl() offers
ATM_SETBACKEND to every registered backend handler with no core-level
"backend already assigned" rejection:
net/atm/ioctl.c:do_vcc_ioctl() {
...
list_for_each(pos, &ioctl_list) {
struct atm_ioctl *ic = list_entry(pos, struct atm_ioctl, list);
if (try_module_get(ic->owner)) {
error = ic->ioctl(sock, cmd, arg);
...
}
so the other writer of user_back runs under the same mutex without any such
guard. Would it make sense to add the same rejection to br2684_regvcc(), or
to do the check once in the shared ATM_SETBACKEND dispatch instead?
> pvcc = kzalloc_obj(*pvcc);
> if (pvcc == NULL)
> return -ENOMEM;
[Severity: Medium]
This isn't a bug introduced by this patch, but since the same field is
involved: is the publication of user_back ordered against the publication of
the push callback? Further down in pppoatm_assign_vcc() the two stores are
plain and adjacent:
net/atm/pppoatm.c:pppoatm_assign_vcc() {
...
atmvcc->user_back = pvcc;
atmvcc->push = pppoatm_push;
atmvcc->pop = pppoatm_pop;
...
}
ATM_SETBACKEND requires sock->state == SS_CONNECTED, and the driver receive
path calls vcc->push(vcc, skb) from softirq context without any lock shared
with do_vcc_ioctl(), which holds only ioctl_mutex. If the store to
atmvcc->push becomes visible first, pppoatm_push() reads user_back as NULL
and dereferences it with no NULL check:
net/atm/pppoatm.c:pppoatm_push() {
struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
pr_debug("\n");
if (skb == NULL) {
...
module = pvcc->old_owner;
...
switch (pvcc->encaps) {
...
}
The sibling backend orders the same sequence explicitly:
net/atm/br2684.c:br2684_regvcc() {
...
brvcc->old_owner = atmvcc->owner;
barrier();
atmvcc->push = br2684_push;
...
}
Would smp_store_release(&atmvcc->push, pppoatm_push) after user_back is set
be appropriate here?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902040320.4007552-1-v3rdant.xiang%40gmail.com
^ permalink raw reply [flat|nested] 2+ messages in thread