RE: [External Mail] Re: [PATCH v4 4/7] net: wwan: t9xx: Add control port

["Wu. JackBB (GSM)" <JackBB_Wu@compal.com>]

Hi Simon,

> > +static void mtk_port_free_or_backup(...)
>
> [Severity: High]
> Does this directly bypass the kref API?

Will fix in v5. The kref count is 1 at this point (the initial
allocation reference only), so the behavior is correct — but
kref_put is the proper API. Will change to
kref_put(&port->kref, mtk_port_release). Same fix applied to
mtk_port_stale_list_grp_cleanup below.

> > +static struct mtk_port *mtk_port_search_by_id(...)
>
> [Severity: High]
> Is it safe to perform radix tree lookups without an RCU read-side
> critical section?

mtk_port_search_by_id is called from the CLDMA workqueue context
(rx_dispatch) and during probe/FSM startup. Port creation and
destruction are driven by FSM state transitions on the FSM
kthread. Ports are created before the CLDMA RX queue is opened,
and the RX queue is stopped before ports are destroyed. No
concurrent modification occurs during radix tree lookups.

The tree was initialized with INIT_RADIX_TREE (not RCU-tagged),
and all modifications are serialized by FSM state ordering.

> > +static struct mtk_port *mtk_port_search_by_name(...)
>
> [Severity: High]
> Does this radix tree iteration require rcu_read_lock() protection?

mtk_port_search_by_name is only called from
mtk_port_internal_open, which is exclusively invoked by the FSM
kthread during mtk_fsm_ctrl_ch_start. Port creation and
destruction are also driven by FSM state transitions on the same
kthread. Since the FSM serializes these operations, no concurrent
port removal can occur while iterating.

> > +static void mtk_port_stale_list_grp_cleanup(...)
>
> [Severity: High]
> This appears to be another instance of bypassing the kref API by
> calling mtk_port_release() directly instead of kref_put().

We will fix in v5:
Use kref_put(&port->kref, mtk_port_release) instead of
calling mtk_port_release() directly. The clear_bit before
it ensures no double list_del, and kref count is 1 so the
behavior is unchanged — but kref_put is the proper API.

> [Severity: Medium]
> Does this leak the s_list->dev_id? The list entries are freed here during
> module unload, but there doesn't appear to be a corresponding call to
> ida_free() to return the dev_id to the allocator.

We will fix in v5,Add ida_free before kfree(s_list) to properly release the
   dev_id:
     if (s_list->dev_id >= 0)
         ida_free(&ccci_dev_ids, s_list->dev_id);
     kfree(s_list);

> > +static int mtk_port_trb_init(...)
>
> [Severity: High]
> Does this assignment need to take a reference to the port with
> kref_get()?

The TRB lifecycle is protected by kref on the TRB itself
(kref_get before submit, kref_put in both the caller and the
completion callback). The port is not freed while TRBs reference
it — port destruction goes through kref_put(&port->kref,
mtk_port_release), and TRB_CMD_DISABLE triggers
mtk_ctrl_ch_flush, which forces all pending TRBs to complete
before the port teardown proceeds.

> > +static int mtk_port_send_data(...)
>
> [Severity: Medium]
> Can concurrent writers corrupt the tx_seq variable?

tx_seq is a per-port 8-bit sequence number with a single writer
per port: external ports are serialized by the WWAN core
ops_lock mutex around tx/tx_blocking callbacks, and internal
ports are written exclusively by the FSM kthread. Concurrent
transmissions on the same port do not occur.

> > +static int mtk_port_rx_dispatch_frag_skb(...)
>
> [Severity: High]
> Can this lead to use-after-free and double-free on the skb?

No. The recv callback (mtk_port_internal_recv) does not free
the skb on error — it returns -ENXIO and leaves skb ownership
with the caller. When recv returns an error, it is safe to
access the skb to restore frag_list. The caller
(mtk_port_rx_dispatch) then frees the skb once in the
drop_data path.

> > +static int mtk_port_rx_dispatch(...)
>
> [Severity: High]
> This is where the second free of the skb occurs when propagating
> the error from the recv callback.

As above, the recv callback does not free the skb on error.
The drop_data path in mtk_port_rx_dispatch is the only place
the skb is freed — no double free occurs.

> > +static void mtk_port_add_header(...)
>
> [Severity: Medium]
> This is the other side of the tx_seq data race.

Same as above — tx_seq is serialized by the WWAN core ops_lock
for external ports and the single-threaded FSM kthread for
internal ports.

> > +static int mtk_port_status_update(...)
>
> [Severity: High]
> Is there a missing bounds check on msg->port_cnt here?

The message is validated by head_pattern, tail_pattern, and
version checks before the loop. The port enumeration protocol
between host and modem guarantees the buffer contains exactly
port_cnt entries. The modem firmware is a trusted source —
this is a control plane interface, not an untrusted network
protocol.

> > +static int mtk_port_ch_enable(...)
>
> [Severity: High]
> Could this create an infinite loop in kernel space?

This is intentional. Channel enable/disable are control plane
operations that must complete or timeout — aborting mid-operation
due to a signal would leave the channel in an inconsistent state.
The loop does not busy-spin: each iteration sleeps in
wait_event_interruptible_timeout until the condition is met or
the timeout (MTK_DFLT_TRB_TIMEOUT) expires. The timeout
guarantees bounded execution.

> > +static int mtk_port_ch_disable(...)
>
> [Severity: High]
> This appears to have the same infinite loop risk.

Same reasoning as mtk_port_ch_enable. The timeout guarantees
bounded execution per iteration.

> > +static int mtk_port_internal_recv(...)
>
> [Severity: High]
> This is where the skb is freed on the error path before returning
> -ENXIO.

mtk_port_internal_recv does not free the skb on the error path.
The drop_data label simply returns -ENXIO without calling
dev_kfree_skb_any(). The skb ownership remains with the caller.

> > +static int mtk_port_common_open(...)
>
> [Severity: High]
> Is this test_bit() and set_bit() sequence intended to provide
> mutual exclusion?

All callers of mtk_port_common_open acquire port_mngr_grp_mtx
via mtk_port_get_locked before calling this function, which
serializes concurrent open attempts on the same port. The
test_bit/set_bit sequence is protected by the mutex.

Thanks.

Jack Wu