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

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

Hi Jakub,

Addressing sashiko AI code review comments for this patch, as
requested by you in the patch 3/7 review:
https://patchwork.kernel.org/project/netdevbpf/patch/20260610-t9xx_driver_v1-v2-3-c65addf23b3f@compal.com/#27006088

Q1: Does this unconditionally free the internal port memory? If an internal
user or an active TRB holds a reference to the port during device teardown,
it seems this bypasses kref_put and directly calls the release function.
This could lead to a use-after-free when they attempt to access the port
or drop their reference.

  Internal ports are exclusively used by the FSM layer. During
  teardown, mtk_fsm_ctrl_ch_stop calls mtk_port_internal_close,
  which calls kref_put to release the user's reference before
  mtk_port_free_or_backup is reached. By the time
  mtk_port_free_or_backup runs, the kref count is 1 (the initial
  allocation reference only). Calling mtk_port_release directly at
  this point is equivalent to kref_put decrementing from 1 to 0.
  Internal ports do not use the stale list backup mechanism, so
  bypassing kref_put avoids the unnecessary stale list check in the
  kref_put path.

Q2: Is it safe to traverse the port_tbl radix tree without holding
rcu_read_lock or the port_mngr_grp_mtx mutex? Concurrent port deletions
could free radix tree nodes or mtk_port structures during traversal,
potentially leading to a use-after-free when the un-refcounted pointers
are dereferenced.

  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 search_by_name is iterating the radix tree.

Q3: If this array allocation fails during teardown, does the function skip
deleting items from the radix tree and freeing the dynamically allocated
mtk_port objects? This appears to bypass the teardown phase entirely and
leak memory.

  Valid. Fixed in v3 by replacing kcalloc + radix_tree_gang_lookup
  with radix_tree_for_each_slot() and single-entry gang_lookup,
  eliminating the allocation in the teardown path.

Q4: Should we increment the port's reference count when assigning it to
trb->priv? If a writer thread is interrupted by a signal while waiting for
hardware completion, it returns and drops the port. If the port is then
closed and freed, the asynchronous callback mtk_port_tx_complete will
dereference the freed port, resulting in a use-after-free.

  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). When a blocking operation times out, the caller releases
  its kref reference, but the TRB remains alive in the transaction
  layer until the completion callback fires. The port is not freed at
  this point — port destruction goes through kref_put(&port->kref,
  mtk_port_release), and the port's reference count is not decremented
  until the close path completes. Additionally, TRB_CMD_DISABLE
  triggers mtk_ctrl_ch_flush in the transaction layer, which forces
  all pending TRBs to complete before the port teardown proceeds.

Q5: Can concurrent packet transmissions on the same port race here? There
does not appear to be any synchronization protecting the read and increment
of port->tx_seq.

  tx_seq is a per-port 8-bit sequence number. Each port has a single
  writer in practice: external ports are written from user space via
  cdev write() (serialized by the file descriptor), and internal
  ports are written exclusively by the FSM kthread. Concurrent
  transmissions on the same port do not occur in normal operation.

Q6: Does this safely handle fragmented SKBs? Without using pskb_may_pull
or similar, the header might span across multiple DMA fragments. If
skb->data does not contain the entire 16-byte header, this cast could
result in reading out-of-bounds memory.

  The RX SKBs are allocated internally by the CLDMA driver using
  __dev_alloc_skb with linear buffers. The CCCI header always resides
  in the contiguous linear data region. mtk_port_strip_header already
  validates skb->len >= sizeof(*ccci_h) before accessing the header.
  Since these SKBs are not received from the network stack and are
  always linear, pskb_may_pull is not needed here.

Q7: Is there a bounds check to ensure that the packet buffer is large
enough to hold the number of ports specified by msg->port_cnt? If the
modem provides an arbitrarily large count, this loop could read past the
end of the packet data.

  The message is already validated by head_pattern, tail_pattern, and
  version checks before the loop. The port enumeration protocol
  between host and modem guarantees that 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.

Q8: If a signal is pending, wait_event_interruptible_timeout will return
-ERESTARTSYS immediately. Jumping back to start_wait without returning to
userspace to handle the signal creates an infinite tight loop that will
stall the CPU at 100% until the hardware completes.

  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 spin: each iteration sleeps in
  wait_event_interruptible_timeout until either the signal re-fires,
  the condition is met, or the timeout expires. The timeout
  (MTK_DFLT_TRB_TIMEOUT) guarantees bounded execution.

Q9: Is there a race condition between checking PORT_S_OPEN and setting it?
If two threads concurrently open the same port, they might both read the
bit as clear and proceed with initialization. Could test_and_set_bit be
used?

  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.

Q10: If mtk_port_common_open fails, the port reference is dropped via
mtk_port_put_locked, but the pointer itself is not set to NULL before
jumping to out. This causes the function to return an un-refcounted,
invalid port pointer.

  Valid. Fixed in v3 by adding port = NULL after mtk_port_put_locked()
  in the error path.

Q11: If mtk_cldma_submit_tx fails with a fatal error (like -EINVAL), the
error path breaks out of the switch without unlinking the SKB from
skb_list and without signaling completion. Will the next processing loop
retrieve the exact same failing SKB with skb_peek and infinitely repeat
the failure?

  Valid. Fixed in v3 by restoring skb_unlink + trb_complete for
  non-EAGAIN errors in the TX path.

Thanks.


================================================================================================================================================================
This message may contain information which is private, privileged or confidential of Compal Electronics, Inc. If you are not the intended recipient of this message, please notify the sender and destroy/delete the message. Any review, retransmission, dissemination or other use of, or taking of any action in reliance upon this information, by persons or entities other than the intended recipient is prohibited.
================================================================================================================================================================