May 2024 - Linux-stable-mirror

[PATCH] xhci: Handle TD clearing for multiple streams case

by Hector Martin

When multiple streams are in use, multiple TDs might be in flight when an endpoint is stopped. We need to issue a Set TR Dequeue Pointer for each, to ensure everything is reset properly and the caches cleared. Change the logic so that any N>1 TDs found active for different streams are deferred until after the first one is processed, calling xhci_invalidate_cancelled_tds() again from xhci_handle_cmd_set_deq() to queue another command until we are done with all of them. Also change the error/"should never happen" paths to ensure we at least clear any affected TDs, even if we can't issue a command to clear the hardware cache, and complain loudly with an xhci_warn() if this ever happens. This problem case dates back to commit e9df17eb1408 ("USB: xhci: Correct assumptions about number of rings per endpoint.") early on in the XHCI driver's life, when stream support was first added. At that point, this condition would cause TDs to not be given back at all, causing hanging transfers - but no security bug. It was then identified but not fixed nor made into a warning in commit 674f8438c121 ("xhci: split handling halted endpoints into two steps"), which added a FIXME comment for the problem case (without materially changing the behavior as far as I can tell, though the new logic made the problem more obvious). Then later, in commit 94f339147fc3 ("xhci: Fix failure to give back some cached cancelled URBs."), it was acknowledged again. This commit was unfortunately not reviewed at all, as it was authored by the maintainer directly. Had it been, perhaps a second set of eyes would've noticed that it does not fix the bug, but rather just makes it (much) worse. It turns the "transfers hang" bug into a "random memory corruption" bug, by blindly marking TDs as complete without actually clearing them at all nor moving the dequeue pointer past them, which means they aren't actually complete, and the xHC will try to transfer data to/from them when the endpoint resumes, now to freed memory buffers. This could have been a legitimate oversight, but apparently the commit author was aware of the problem (yet still chose to submit it): It was still mentioned as a FIXME, an xhci_dbg() was added to log the problem condition, and the remaining issue was mentioned in the commit description. The choice of making the log type xhci_dbg() for what is, at this point, a completely unhandled and known broken condition is puzzling and unfortunate, as it guarantees that no actual users would see the log in production, thereby making it nigh undebuggable (indeed, even if you turn on DEBUG, the message doesn't really hint at there being a problem at all). It took me *months* of random xHC crashes to finally find a reliable repro and be able to do a deep dive debug session, which could all have been avoided had this unhandled, broken condition been actually reported with a warning, as it should have been as a bug intentionally left in unfixed (never mind that it shouldn't have been left in at all). > Another fix to solve clearing the caches of all stream rings with > cancelled TDs is needed, but not as urgent. 3 years after that statement and 14 years after the original bug was introduced, I think it's finally time to fix it. And maybe next time let's not leave bugs unfixed (that are actually worse than the original bug), and let's actually get people to review kernel commits please. Fixes xHC crashes and IOMMU faults with UAS devices when handling errors/faults. Easiest repro is to use `hdparm` to mark an early sector (e.g. 1024) on a disk as bad, then `cat /dev/sdX > /dev/null` in a loop. At least in the case of JMicron controllers, the read errors end up having to cancel two TDs (for two queued requests to different streams) and the one that didn't get cleared properly ends up faulting the xHC entirely when it tries to access DMA pages that have since been unmapped, referred to by the stale TDs. This normally happens quickly (after two or three loops). After this fix, I left the `cat` in a loop running overnight and experienced no xHC failures, with all read errors recovered properly. Repro'd and tested on an Apple M1 Mac Mini (dwc3 host). On systems without an IOMMU, this bug would instead silently corrupt freed memory, making this a security bug (even on systems with IOMMUs this could silently corrupt memory belonging to other USB devices on the same controller, so it's still a security bug). Given that the kernel autoprobes partition tables, I'm pretty sure a malicious USB device pretending to be a UAS device and reporting an error with the right timing could deliberately trigger a UAF and write to freed memory, with no user action. Fixes: e9df17eb1408 ("USB: xhci: Correct assumptions about number of rings per endpoint.") Fixes: 94f339147fc3 ("xhci: Fix failure to give back some cached cancelled URBs.") Fixes: 674f8438c121 ("xhci: split handling halted endpoints into two steps") Cc: stable(a)vger.kernel.org Cc: security(a)kernel.org Signed-off-by: Hector Martin <marcan(a)marcan.st> --- drivers/usb/host/xhci-ring.c | 54 +++++++++++++++++++++++++++++++++++--------- drivers/usb/host/xhci.h | 1 + 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 575f0fd9c9f1..9c06502be098 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -1034,13 +1034,27 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) break; case TD_DIRTY: /* TD is cached, clear it */ case TD_HALTED: + case TD_CLEARING_CACHE_DEFERRED: + if (cached_td) { + if (cached_td->urb->stream_id != td->urb->stream_id) { + /* Multiple streams case, defer move dq */ + xhci_dbg(xhci, + "Move dq deferred: stream %u URB %p\n", + td->urb->stream_id, td->urb); + td->cancel_status = TD_CLEARING_CACHE_DEFERRED; + break; + } + + /* Should never happen, at least try to clear the TD if it does */ + xhci_warn(xhci, + "Found multiple active URBs %p and %p in stream %u?\n", + td->urb, cached_td->urb, + td->urb->stream_id); + td_to_noop(xhci, ring, cached_td, false); + cached_td->cancel_status = TD_CLEARED; + } + td->cancel_status = TD_CLEARING_CACHE; - if (cached_td) - /* FIXME stream case, several stopped rings */ - xhci_dbg(xhci, - "Move dq past stream %u URB %p instead of stream %u URB %p\n", - td->urb->stream_id, td->urb, - cached_td->urb->stream_id, cached_td->urb); cached_td = td; break; } @@ -1060,10 +1074,16 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) if (err) { /* Failed to move past cached td, just set cached TDs to no-op */ list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) { - if (td->cancel_status != TD_CLEARING_CACHE) + /* + * Deferred TDs need to have the deq pointer set after the above command + * completes, so if that failed we just give up on all of them (and + * complain loudly since this could cause issues due to caching). + */ + if (td->cancel_status != TD_CLEARING_CACHE && + td->cancel_status != TD_CLEARING_CACHE_DEFERRED) continue; - xhci_dbg(xhci, "Failed to clear cancelled cached URB %p, mark clear anyway\n", - td->urb); + xhci_warn(xhci, "Failed to clear cancelled cached URB %p, mark clear anyway\n", + td->urb); td_to_noop(xhci, ring, td, false); td->cancel_status = TD_CLEARED; } @@ -1350,6 +1370,7 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, struct xhci_ep_ctx *ep_ctx; struct xhci_slot_ctx *slot_ctx; struct xhci_td *td, *tmp_td; + bool deferred = false; ep_index = TRB_TO_EP_INDEX(le32_to_cpu(trb->generic.field[3])); stream_id = TRB_TO_STREAM_ID(le32_to_cpu(trb->generic.field[2])); @@ -1436,6 +1457,8 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, xhci_dbg(ep->xhci, "%s: Giveback cancelled URB %p TD\n", __func__, td->urb); xhci_td_cleanup(ep->xhci, td, ep_ring, td->status); + } else if (td->cancel_status == TD_CLEARING_CACHE_DEFERRED) { + deferred = true; } else { xhci_dbg(ep->xhci, "%s: Keep cancelled URB %p TD as cancel_status is %d\n", __func__, td->urb, td->cancel_status); @@ -1445,8 +1468,17 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, ep->ep_state &= ~SET_DEQ_PENDING; ep->queued_deq_seg = NULL; ep->queued_deq_ptr = NULL; - /* Restart any rings with pending URBs */ - ring_doorbell_for_active_rings(xhci, slot_id, ep_index); + + if (deferred) { + /* We have more streams to clear */ + xhci_dbg(ep->xhci, "%s: Pending TDs to clear, continuing with invalidation\n", + __func__); + xhci_invalidate_cancelled_tds(ep); + } else { + /* Restart any rings with pending URBs */ + xhci_dbg(ep->xhci, "%s: All TDs cleared, ring doorbell\n", __func__); + ring_doorbell_for_active_rings(xhci, slot_id, ep_index); + } } static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id, diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 6f4bf98a6282..aa4379bdb90c 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1276,6 +1276,7 @@ enum xhci_cancelled_td_status { TD_DIRTY = 0, TD_HALTED, TD_CLEARING_CACHE, + TD_CLEARING_CACHE_DEFERRED, TD_CLEARED, }; --- base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6 change-id: 20240524-xhci-streams-124e88db52e6 Best regards, -- Hector Martin <marcan(a)marcan.st>

1 year, 1 month

3
2
0 0

[PATCH v3] af_packet: Handle outgoing VLAN packets without hardware offloading

by Chengen Du

The issue initially stems from libpcap [1]. In the outbound packet path, if hardware VLAN offloading is unavailable, the VLAN tag is inserted into the payload but then cleared from the sk_buff struct. Consequently, this can lead to a false negative when checking for the presence of a VLAN tag, causing the packet sniffing outcome to lack VLAN tag information (i.e., TCI-TPID). As a result, the packet capturing tool may be unable to parse packets as expected. The TCI-TPID is missing because the prb_fill_vlan_info() function does not modify the tp_vlan_tci/tp_vlan_tpid values, as the information is in the payload and not in the sk_buff struct. The skb_vlan_tag_present() function only checks vlan_all in the sk_buff struct. In cooked mode, the L2 header is stripped, preventing the packet capturing tool from determining the correct TCI-TPID value. Additionally, the protocol in SLL is incorrect, which means the packet capturing tool cannot parse the L3 header correctly. [1] https://github.com/the-tcpdump-group/libpcap/issues/1105 Fixes: f6fb8f100b80 ("af-packet: TPACKET_V3 flexible buffer implementation.") Cc: stable(a)vger.kernel.org Signed-off-by: Chengen Du <chengen.du(a)canonical.com> --- net/packet/af_packet.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index ea3ebc160e25..82b36e90d73b 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -1011,6 +1011,10 @@ static void prb_fill_vlan_info(struct tpacket_kbdq_core *pkc, ppd->hv1.tp_vlan_tci = skb_vlan_tag_get(pkc->skb); ppd->hv1.tp_vlan_tpid = ntohs(pkc->skb->vlan_proto); ppd->tp_status = TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; + } else if (eth_type_vlan(pkc->skb->protocol)) { + ppd->hv1.tp_vlan_tci = ntohs(vlan_eth_hdr(pkc->skb)->h_vlan_TCI); + ppd->hv1.tp_vlan_tpid = ntohs(pkc->skb->protocol); + ppd->tp_status = TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; } else { ppd->hv1.tp_vlan_tci = 0; ppd->hv1.tp_vlan_tpid = 0; @@ -2428,6 +2432,10 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, h.h2->tp_vlan_tci = skb_vlan_tag_get(skb); h.h2->tp_vlan_tpid = ntohs(skb->vlan_proto); status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; + } else if (eth_type_vlan(skb->protocol)) { + h.h2->tp_vlan_tci = ntohs(vlan_eth_hdr(skb)->h_vlan_TCI); + h.h2->tp_vlan_tpid = ntohs(skb->protocol); + status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; } else { h.h2->tp_vlan_tci = 0; h.h2->tp_vlan_tpid = 0; @@ -2457,7 +2465,8 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, sll->sll_halen = dev_parse_header(skb, sll->sll_addr); sll->sll_family = AF_PACKET; sll->sll_hatype = dev->type; - sll->sll_protocol = skb->protocol; + sll->sll_protocol = (skb->protocol == htons(ETH_P_8021Q)) ? + vlan_eth_hdr(skb)->h_vlan_encapsulated_proto : skb->protocol; sll->sll_pkttype = skb->pkt_type; if (unlikely(packet_sock_flag(po, PACKET_SOCK_ORIGDEV))) sll->sll_ifindex = orig_dev->ifindex; @@ -3482,7 +3491,8 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, /* Original length was stored in sockaddr_ll fields */ origlen = PACKET_SKB_CB(skb)->sa.origlen; sll->sll_family = AF_PACKET; - sll->sll_protocol = skb->protocol; + sll->sll_protocol = (skb->protocol == htons(ETH_P_8021Q)) ? + vlan_eth_hdr(skb)->h_vlan_encapsulated_proto : skb->protocol; } sock_recv_cmsgs(msg, sk, skb); @@ -3539,6 +3549,10 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, aux.tp_vlan_tci = skb_vlan_tag_get(skb); aux.tp_vlan_tpid = ntohs(skb->vlan_proto); aux.tp_status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; + } else if (eth_type_vlan(skb->protocol)) { + aux.tp_vlan_tci = ntohs(vlan_eth_hdr(skb)->h_vlan_TCI); + aux.tp_vlan_tpid = ntohs(skb->protocol); + aux.tp_status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; } else { aux.tp_vlan_tci = 0; aux.tp_vlan_tpid = 0; -- 2.40.1

1 year, 1 month

1
0
0 0

[PATCH v2] af_packet: Handle outgoing VLAN packets without hardware offloading

by Chengen Du

The issue initially stems from libpcap [1]. In the outbound packet path, if hardware VLAN offloading is unavailable, the VLAN tag is inserted into the payload but then cleared from the sk_buff struct. Consequently, this can lead to a false negative when checking for the presence of a VLAN tag, causing the packet sniffing outcome to lack VLAN tag information (i.e., TCI-TPID). As a result, the packet capturing tool may be unable to parse packets as expected. The TCI-TPID is missing because the prb_fill_vlan_info() function does not modify the tp_vlan_tci/tp_vlan_tpid values, as the information is in the payload and not in the sk_buff struct. The skb_vlan_tag_present() function only checks vlan_all in the sk_buff struct. In cooked mode, the L2 header is stripped, preventing the packet capturing tool from determining the correct TCI-TPID value. Additionally, the protocol in SLL is incorrect, which means the packet capturing tool cannot parse the L3 header correctly. [1] https://github.com/the-tcpdump-group/libpcap/issues/1105 Fixes: f6fb8f100b80 ("af-packet: TPACKET_V3 flexible buffer implementation.") Signed-off-by: Chengen Du <chengen.du(a)canonical.com> --- net/packet/af_packet.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index ea3ebc160e25..82b36e90d73b 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -1011,6 +1011,10 @@ static void prb_fill_vlan_info(struct tpacket_kbdq_core *pkc, ppd->hv1.tp_vlan_tci = skb_vlan_tag_get(pkc->skb); ppd->hv1.tp_vlan_tpid = ntohs(pkc->skb->vlan_proto); ppd->tp_status = TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; + } else if (eth_type_vlan(pkc->skb->protocol)) { + ppd->hv1.tp_vlan_tci = ntohs(vlan_eth_hdr(pkc->skb)->h_vlan_TCI); + ppd->hv1.tp_vlan_tpid = ntohs(pkc->skb->protocol); + ppd->tp_status = TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; } else { ppd->hv1.tp_vlan_tci = 0; ppd->hv1.tp_vlan_tpid = 0; @@ -2428,6 +2432,10 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, h.h2->tp_vlan_tci = skb_vlan_tag_get(skb); h.h2->tp_vlan_tpid = ntohs(skb->vlan_proto); status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; + } else if (eth_type_vlan(skb->protocol)) { + h.h2->tp_vlan_tci = ntohs(vlan_eth_hdr(skb)->h_vlan_TCI); + h.h2->tp_vlan_tpid = ntohs(skb->protocol); + status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; } else { h.h2->tp_vlan_tci = 0; h.h2->tp_vlan_tpid = 0; @@ -2457,7 +2465,8 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, sll->sll_halen = dev_parse_header(skb, sll->sll_addr); sll->sll_family = AF_PACKET; sll->sll_hatype = dev->type; - sll->sll_protocol = skb->protocol; + sll->sll_protocol = (skb->protocol == htons(ETH_P_8021Q)) ? + vlan_eth_hdr(skb)->h_vlan_encapsulated_proto : skb->protocol; sll->sll_pkttype = skb->pkt_type; if (unlikely(packet_sock_flag(po, PACKET_SOCK_ORIGDEV))) sll->sll_ifindex = orig_dev->ifindex; @@ -3482,7 +3491,8 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, /* Original length was stored in sockaddr_ll fields */ origlen = PACKET_SKB_CB(skb)->sa.origlen; sll->sll_family = AF_PACKET; - sll->sll_protocol = skb->protocol; + sll->sll_protocol = (skb->protocol == htons(ETH_P_8021Q)) ? + vlan_eth_hdr(skb)->h_vlan_encapsulated_proto : skb->protocol; } sock_recv_cmsgs(msg, sk, skb); @@ -3539,6 +3549,10 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, aux.tp_vlan_tci = skb_vlan_tag_get(skb); aux.tp_vlan_tpid = ntohs(skb->vlan_proto); aux.tp_status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; + } else if (eth_type_vlan(skb->protocol)) { + aux.tp_vlan_tci = ntohs(vlan_eth_hdr(skb)->h_vlan_TCI); + aux.tp_vlan_tpid = ntohs(skb->protocol); + aux.tp_status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID; } else { aux.tp_vlan_tci = 0; aux.tp_vlan_tpid = 0; -- 2.40.1

1 year, 1 month

2
2
0 0

RE: [PATCH] Bluetooth: btnxpuart: Shutdown timer and prevent rearming when driver unloading

by Bough Chen

> -----Original Message----- > From: Luke Wang <ziniu.wang_1(a)nxp.com> > Sent: 2024年5月17日 19:16 > To: marcel(a)holtmann.org; luiz.dentz(a)gmail.com > Cc: linux-bluetooth(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; Amitkumar > Karwar <amitkumar.karwar(a)nxp.com>; Rohit Fule <rohit.fule(a)nxp.com>; > Neeraj Sanjay Kale <neeraj.sanjaykale(a)nxp.com>; Sherry Sun > <sherry.sun(a)nxp.com>; Bough Chen <haibo.chen(a)nxp.com>; Jun Li > <jun.li(a)nxp.com>; Guillaume Legoupil <guillaume.legoupil(a)nxp.com>; Salim > Chebbo <salim.chebbo(a)nxp.com>; imx(a)lists.linux.dev > Subject: [PATCH] Bluetooth: btnxpuart: Shutdown timer and prevent rearming > when driver unloading > > From: Luke Wang <ziniu.wang_1(a)nxp.com> > > When unload the btnxpuart driver, its associated timer will be deleted. > If the timer happens to be modified at this moment, it leads to the kernel call > this timer even after the driver unloaded, resulting in kernel panic. > Use timer_shutdown_sync() instead of del_timer_sync() to prevent rearming. > > panic log: > Internal error: Oops: 0000000086000007 [#1] PREEMPT SMP > Modules linked in: algif_hash algif_skcipher af_alg moal(O) mlan(O) > crct10dif_ce polyval_ce polyval_generic snd_soc_imx_card > snd_soc_fsl_asoc_card snd_soc_imx_audmux mxc_jpeg_encdec v4l2_jpeg > snd_soc_wm8962 snd_soc_fsl_micfil snd_soc_fsl_sai flexcan snd_soc_fsl_utils > ap130x rpmsg_ctrl imx_pcm_dma can_dev rpmsg_char pwm_fan fuse [last > unloaded: btnxpuart] > CPU: 5 PID: 723 Comm: memtester Tainted: G O > 6.6.23-lts-next-06207-g4aef2658ac28 #1 > Hardware name: NXP i.MX95 19X19 board (DT) > pstate: 20400009 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : 0xffff80007a2cf464 > lr : call_timer_fn.isra.0+0x24/0x80 > ... > Call trace: > 0xffff80007a2cf464 > __run_timers+0x234/0x280 > run_timer_softirq+0x20/0x40 > __do_softirq+0x100/0x26c > ____do_softirq+0x10/0x1c > call_on_irq_stack+0x24/0x4c > do_softirq_own_stack+0x1c/0x2c > irq_exit_rcu+0xc0/0xdc > el0_interrupt+0x54/0xd8 > __el0_irq_handler_common+0x18/0x24 > el0t_64_irq_handler+0x10/0x1c > el0t_64_irq+0x190/0x194 > Code: ???????? ???????? ???????? ???????? (????????) > ---[ end trace 0000000000000000 ]--- > Kernel panic - not syncing: Oops: Fatal exception in interrupt > SMP: stopping secondary CPUs > Kernel Offset: disabled > CPU features: 0x0,c0000000,40028143,1000721b > Memory Limit: none > ---[ end Kernel panic - not syncing: Oops: Fatal exception in interrupt ]--- Hi all, This patch is already accepted, but I notice it lack fix tag, can anyone help add the following fix tag? Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets") Cc: stable(a)vger.kernel.org This patch should also put into stable tree. I add the stable tree mail list here. Best Regards Haibo Chen > > Signed-off-by: Luke Wang <ziniu.wang_1(a)nxp.com> > --- > drivers/bluetooth/btnxpuart.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c index > 7f88b6f52f26..77f974a5251b 100644 > --- a/drivers/bluetooth/btnxpuart.c > +++ b/drivers/bluetooth/btnxpuart.c > @@ -328,7 +328,7 @@ static void ps_cancel_timer(struct btnxpuart_dev > *nxpdev) > struct ps_data *psdata = &nxpdev->psdata; > > flush_work(&psdata->work); > - del_timer_sync(&psdata->ps_timer); > + timer_shutdown_sync(&psdata->ps_timer); > } > > static void ps_control(struct hci_dev *hdev, u8 ps_state) > -- > 2.34.1

1 year, 1 month

1
0
0 0

Re: Patch "platform/x86: xiaomi-wmi: Fix race condition when reporting key events" has been added to the 5.4-stable tree

by Armin Wolf

Am 26.05.24 um 22:19 schrieb Sasha Levin: > This is a note to let you know that I've just added the patch titled > > platform/x86: xiaomi-wmi: Fix race condition when reporting key events > > to the 5.4-stable tree which can be found at: > http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum… > > The filename of the patch is: > platform-x86-xiaomi-wmi-fix-race-condition-when-repo.patch > and it can be found in the queue-5.4 subdirectory. > > If you, or anyone else, feels it should not be added to the stable tree, > please let <stable(a)vger.kernel.org> know about it. > Hi, the underlying race condition can only be triggered since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on all CPUs"), which afaik was introduced with kernel 6.8. Because of this, i do not think that we have to backport this commit to kernels before 6.8. Thanks, Armin Wolf > > commit 7217162b48f60edc29afbeff641b7de02076bb86 > Author: Armin Wolf <W_Armin(a)gmx.de> > Date: Tue Apr 2 16:30:57 2024 +0200 > > platform/x86: xiaomi-wmi: Fix race condition when reporting key events > > [ Upstream commit 290680c2da8061e410bcaec4b21584ed951479af ] > > Multiple WMI events can be received concurrently, so multiple instances > of xiaomi_wmi_notify() can be active at the same time. Since the input > device is shared between those handlers, the key input sequence can be > disturbed. > > Fix this by protecting the key input sequence with a mutex. > > Compile-tested only. > > Fixes: edb73f4f0247 ("platform/x86: wmi: add Xiaomi WMI key driver") > Signed-off-by: Armin Wolf <W_Armin(a)gmx.de> > Link: https://lore.kernel.org/r/20240402143059.8456-2-W_Armin@gmx.de > Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy(a)linux.intel.com> > Reviewed-by: Hans de Goede <hdegoede(a)redhat.com> > Signed-off-by: Hans de Goede <hdegoede(a)redhat.com> > Signed-off-by: Sasha Levin <sashal(a)kernel.org> > > diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c > index 54a2546bb93bf..be80f0bda9484 100644 > --- a/drivers/platform/x86/xiaomi-wmi.c > +++ b/drivers/platform/x86/xiaomi-wmi.c > @@ -2,8 +2,10 @@ > /* WMI driver for Xiaomi Laptops */ > > #include <linux/acpi.h> > +#include <linux/device.h> > #include <linux/input.h> > #include <linux/module.h> > +#include <linux/mutex.h> > #include <linux/wmi.h> > > #include <uapi/linux/input-event-codes.h> > @@ -20,12 +22,21 @@ > > struct xiaomi_wmi { > struct input_dev *input_dev; > + struct mutex key_lock; /* Protects the key event sequence */ > unsigned int key_code; > }; > > +static void xiaomi_mutex_destroy(void *data) > +{ > + struct mutex *lock = data; > + > + mutex_destroy(lock); > +} > + > static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) > { > struct xiaomi_wmi *data; > + int ret; > > if (wdev == NULL || context == NULL) > return -EINVAL; > @@ -35,6 +46,11 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) > return -ENOMEM; > dev_set_drvdata(&wdev->dev, data); > > + mutex_init(&data->key_lock); > + ret = devm_add_action_or_reset(&wdev->dev, xiaomi_mutex_destroy, &data->key_lock); > + if (ret < 0) > + return ret; > + > data->input_dev = devm_input_allocate_device(&wdev->dev); > if (data->input_dev == NULL) > return -ENOMEM; > @@ -59,10 +75,12 @@ static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy) > if (data == NULL) > return; > > + mutex_lock(&data->key_lock); > input_report_key(data->input_dev, data->key_code, 1); > input_sync(data->input_dev); > input_report_key(data->input_dev, data->key_code, 0); > input_sync(data->input_dev); > + mutex_unlock(&data->key_lock); > } > > static const struct wmi_device_id xiaomi_wmi_id_table[] = {

1 year, 1 month

1
0
0 0

Re: Patch "platform/x86: xiaomi-wmi: Fix race condition when reporting key events" has been added to the 5.10-stable tree

by Armin Wolf

Am 26.05.24 um 22:14 schrieb Sasha Levin: > This is a note to let you know that I've just added the patch titled > > platform/x86: xiaomi-wmi: Fix race condition when reporting key events > > to the 5.10-stable tree which can be found at: > http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum… > > The filename of the patch is: > platform-x86-xiaomi-wmi-fix-race-condition-when-repo.patch > and it can be found in the queue-5.10 subdirectory. > > If you, or anyone else, feels it should not be added to the stable tree, > please let <stable(a)vger.kernel.org> know about it. > Hi, the underlying race condition can only be triggered since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on all CPUs"), which afaik was introduced with kernel 6.8. Because of this, i do not think that we have to backport this commit to kernels before 6.8. Thanks, Armin Wolf > > commit 6f4e7901c3ed3c0bd3da7af5854dbb765fad2e00 > Author: Armin Wolf <W_Armin(a)gmx.de> > Date: Tue Apr 2 16:30:57 2024 +0200 > > platform/x86: xiaomi-wmi: Fix race condition when reporting key events > > [ Upstream commit 290680c2da8061e410bcaec4b21584ed951479af ] > > Multiple WMI events can be received concurrently, so multiple instances > of xiaomi_wmi_notify() can be active at the same time. Since the input > device is shared between those handlers, the key input sequence can be > disturbed. > > Fix this by protecting the key input sequence with a mutex. > > Compile-tested only. > > Fixes: edb73f4f0247 ("platform/x86: wmi: add Xiaomi WMI key driver") > Signed-off-by: Armin Wolf <W_Armin(a)gmx.de> > Link: https://lore.kernel.org/r/20240402143059.8456-2-W_Armin@gmx.de > Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy(a)linux.intel.com> > Reviewed-by: Hans de Goede <hdegoede(a)redhat.com> > Signed-off-by: Hans de Goede <hdegoede(a)redhat.com> > Signed-off-by: Sasha Levin <sashal(a)kernel.org> > > diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c > index 54a2546bb93bf..be80f0bda9484 100644 > --- a/drivers/platform/x86/xiaomi-wmi.c > +++ b/drivers/platform/x86/xiaomi-wmi.c > @@ -2,8 +2,10 @@ > /* WMI driver for Xiaomi Laptops */ > > #include <linux/acpi.h> > +#include <linux/device.h> > #include <linux/input.h> > #include <linux/module.h> > +#include <linux/mutex.h> > #include <linux/wmi.h> > > #include <uapi/linux/input-event-codes.h> > @@ -20,12 +22,21 @@ > > struct xiaomi_wmi { > struct input_dev *input_dev; > + struct mutex key_lock; /* Protects the key event sequence */ > unsigned int key_code; > }; > > +static void xiaomi_mutex_destroy(void *data) > +{ > + struct mutex *lock = data; > + > + mutex_destroy(lock); > +} > + > static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) > { > struct xiaomi_wmi *data; > + int ret; > > if (wdev == NULL || context == NULL) > return -EINVAL; > @@ -35,6 +46,11 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) > return -ENOMEM; > dev_set_drvdata(&wdev->dev, data); > > + mutex_init(&data->key_lock); > + ret = devm_add_action_or_reset(&wdev->dev, xiaomi_mutex_destroy, &data->key_lock); > + if (ret < 0) > + return ret; > + > data->input_dev = devm_input_allocate_device(&wdev->dev); > if (data->input_dev == NULL) > return -ENOMEM; > @@ -59,10 +75,12 @@ static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy) > if (data == NULL) > return; > > + mutex_lock(&data->key_lock); > input_report_key(data->input_dev, data->key_code, 1); > input_sync(data->input_dev); > input_report_key(data->input_dev, data->key_code, 0); > input_sync(data->input_dev); > + mutex_unlock(&data->key_lock); > } > > static const struct wmi_device_id xiaomi_wmi_id_table[] = {

1 year, 1 month

1
0
0 0

Re: Patch "platform/x86: xiaomi-wmi: Fix race condition when reporting key events" has been added to the 5.15-stable tree

by Armin Wolf

Am 26.05.24 um 22:07 schrieb Sasha Levin: > This is a note to let you know that I've just added the patch titled > > platform/x86: xiaomi-wmi: Fix race condition when reporting key events > > to the 5.15-stable tree which can be found at: > http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum… > > The filename of the patch is: > platform-x86-xiaomi-wmi-fix-race-condition-when-repo.patch > and it can be found in the queue-5.15 subdirectory. > > If you, or anyone else, feels it should not be added to the stable tree, > please let <stable(a)vger.kernel.org> know about it. > Hi, the underlying race condition can only be triggered since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on all CPUs"), which afaik was introduced with kernel 6.8. Because of this, i do not think that we have to backport this commit to kernels before 6.8. Thanks, Armin Wolf > > commit 1f436551dd453c28c23f800e7273136e526197cb > Author: Armin Wolf <W_Armin(a)gmx.de> > Date: Tue Apr 2 16:30:57 2024 +0200 > > platform/x86: xiaomi-wmi: Fix race condition when reporting key events > > [ Upstream commit 290680c2da8061e410bcaec4b21584ed951479af ] > > Multiple WMI events can be received concurrently, so multiple instances > of xiaomi_wmi_notify() can be active at the same time. Since the input > device is shared between those handlers, the key input sequence can be > disturbed. > > Fix this by protecting the key input sequence with a mutex. > > Compile-tested only. > > Fixes: edb73f4f0247 ("platform/x86: wmi: add Xiaomi WMI key driver") > Signed-off-by: Armin Wolf <W_Armin(a)gmx.de> > Link: https://lore.kernel.org/r/20240402143059.8456-2-W_Armin@gmx.de > Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy(a)linux.intel.com> > Reviewed-by: Hans de Goede <hdegoede(a)redhat.com> > Signed-off-by: Hans de Goede <hdegoede(a)redhat.com> > Signed-off-by: Sasha Levin <sashal(a)kernel.org> > > diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c > index 54a2546bb93bf..be80f0bda9484 100644 > --- a/drivers/platform/x86/xiaomi-wmi.c > +++ b/drivers/platform/x86/xiaomi-wmi.c > @@ -2,8 +2,10 @@ > /* WMI driver for Xiaomi Laptops */ > > #include <linux/acpi.h> > +#include <linux/device.h> > #include <linux/input.h> > #include <linux/module.h> > +#include <linux/mutex.h> > #include <linux/wmi.h> > > #include <uapi/linux/input-event-codes.h> > @@ -20,12 +22,21 @@ > > struct xiaomi_wmi { > struct input_dev *input_dev; > + struct mutex key_lock; /* Protects the key event sequence */ > unsigned int key_code; > }; > > +static void xiaomi_mutex_destroy(void *data) > +{ > + struct mutex *lock = data; > + > + mutex_destroy(lock); > +} > + > static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) > { > struct xiaomi_wmi *data; > + int ret; > > if (wdev == NULL || context == NULL) > return -EINVAL; > @@ -35,6 +46,11 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) > return -ENOMEM; > dev_set_drvdata(&wdev->dev, data); > > + mutex_init(&data->key_lock); > + ret = devm_add_action_or_reset(&wdev->dev, xiaomi_mutex_destroy, &data->key_lock); > + if (ret < 0) > + return ret; > + > data->input_dev = devm_input_allocate_device(&wdev->dev); > if (data->input_dev == NULL) > return -ENOMEM; > @@ -59,10 +75,12 @@ static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy) > if (data == NULL) > return; > > + mutex_lock(&data->key_lock); > input_report_key(data->input_dev, data->key_code, 1); > input_sync(data->input_dev); > input_report_key(data->input_dev, data->key_code, 0); > input_sync(data->input_dev); > + mutex_unlock(&data->key_lock); > } > > static const struct wmi_device_id xiaomi_wmi_id_table[] = {

1 year, 1 month

1
0
0 0

Re: Patch "platform/x86: xiaomi-wmi: Fix race condition when reporting key events" has been added to the 6.1-stable tree

by Armin Wolf

Am 26.05.24 um 21:57 schrieb Sasha Levin: > This is a note to let you know that I've just added the patch titled > > platform/x86: xiaomi-wmi: Fix race condition when reporting key events > > to the 6.1-stable tree which can be found at: > http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum… > > The filename of the patch is: > platform-x86-xiaomi-wmi-fix-race-condition-when-repo.patch > and it can be found in the queue-6.1 subdirectory. > > If you, or anyone else, feels it should not be added to the stable tree, > please let <stable(a)vger.kernel.org> know about it. > Hi, the underlying race condition can only be triggered since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on all CPUs"), which afaik was introduced with kernel 6.8. Because of this, i do not think that we have to backport this commit to kernels before 6.8. Thanks, Armin Wolf > > commit 1abdef69265133db29772ed5cefea2338f8ce173 > Author: Armin Wolf <W_Armin(a)gmx.de> > Date: Tue Apr 2 16:30:57 2024 +0200 > > platform/x86: xiaomi-wmi: Fix race condition when reporting key events > > [ Upstream commit 290680c2da8061e410bcaec4b21584ed951479af ] > > Multiple WMI events can be received concurrently, so multiple instances > of xiaomi_wmi_notify() can be active at the same time. Since the input > device is shared between those handlers, the key input sequence can be > disturbed. > > Fix this by protecting the key input sequence with a mutex. > > Compile-tested only. > > Fixes: edb73f4f0247 ("platform/x86: wmi: add Xiaomi WMI key driver") > Signed-off-by: Armin Wolf <W_Armin(a)gmx.de> > Link: https://lore.kernel.org/r/20240402143059.8456-2-W_Armin@gmx.de > Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy(a)linux.intel.com> > Reviewed-by: Hans de Goede <hdegoede(a)redhat.com> > Signed-off-by: Hans de Goede <hdegoede(a)redhat.com> > Signed-off-by: Sasha Levin <sashal(a)kernel.org> > > diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c > index 54a2546bb93bf..be80f0bda9484 100644 > --- a/drivers/platform/x86/xiaomi-wmi.c > +++ b/drivers/platform/x86/xiaomi-wmi.c > @@ -2,8 +2,10 @@ > /* WMI driver for Xiaomi Laptops */ > > #include <linux/acpi.h> > +#include <linux/device.h> > #include <linux/input.h> > #include <linux/module.h> > +#include <linux/mutex.h> > #include <linux/wmi.h> > > #include <uapi/linux/input-event-codes.h> > @@ -20,12 +22,21 @@ > > struct xiaomi_wmi { > struct input_dev *input_dev; > + struct mutex key_lock; /* Protects the key event sequence */ > unsigned int key_code; > }; > > +static void xiaomi_mutex_destroy(void *data) > +{ > + struct mutex *lock = data; > + > + mutex_destroy(lock); > +} > + > static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) > { > struct xiaomi_wmi *data; > + int ret; > > if (wdev == NULL || context == NULL) > return -EINVAL; > @@ -35,6 +46,11 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) > return -ENOMEM; > dev_set_drvdata(&wdev->dev, data); > > + mutex_init(&data->key_lock); > + ret = devm_add_action_or_reset(&wdev->dev, xiaomi_mutex_destroy, &data->key_lock); > + if (ret < 0) > + return ret; > + > data->input_dev = devm_input_allocate_device(&wdev->dev); > if (data->input_dev == NULL) > return -ENOMEM; > @@ -59,10 +75,12 @@ static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy) > if (data == NULL) > return; > > + mutex_lock(&data->key_lock); > input_report_key(data->input_dev, data->key_code, 1); > input_sync(data->input_dev); > input_report_key(data->input_dev, data->key_code, 0); > input_sync(data->input_dev); > + mutex_unlock(&data->key_lock); > } > > static const struct wmi_device_id xiaomi_wmi_id_table[] = {

1 year, 1 month

1
0
0 0

"ata: libata: move ata_{port,link,dev}_dbg to standard pr_XXX() macros" - 742bef476ca5352b16063161fb73a56629a6d995 changed logging behavior and disabled a number of messages

by Krzysztof Olędzki

Hi, I noticed that since https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?… (which also got backported to -stable kernels) several of messages from dmesg regarding the ATA subsystem are no longer logged. For example, on my Dell PowerEdge 840 which has only one PATA port I used to get: scsi host1: ata_piix ata1: PATA max UDMA/100 cmd 0x1f0 ctl 0x3f6 bmdma 0xfc00 irq 14 ata2: PATA max UDMA/100 cmd 0x170 ctl 0x376 bmdma 0xfc08 irq 15 ata_piix 0000:00:1f.2: MAP [ P0 P2 P1 P3 ] ata2: port disabled--ignoring ata1.01: NODEV after polling detection ata1.00: ATAPI: SAMSUNG CD-R/RW SW-248F, R602, max UDMA/33 After that commit, the following two log entries are missing: ata2: port disabled--ignoring ata1.01: NODEV after polling detection Note that these are just examples, there are many more messages impacted by that. Looking at the code, these messages are logged via ata_link_dbg / ata_dev_dbg: ata_link_dbg(link, "port disabled--ignoring\n"); [in drivers/ata/libata-eh.c] ata_dev_dbg(dev, "NODEV after polling detection\n"); [in drivers/ata/libata-core.c] The commit change how the logging is called - ata_dev_printk function which was calling printk() directly got replaced with the following macro: +#define ata_dev_printk(level, dev, fmt, ...) \ + pr_ ## level("ata%u.%02u: " fmt, \ + (dev)->link->ap->print_id, \ + (dev)->link->pmp + (dev)->devno, \ + ##__VA_ARGS__) (...) #define ata_link_dbg(link, fmt, ...) \ - ata_link_printk(link, KERN_DEBUG, fmt, ##__VA_ARGS__) + ata_link_printk(debug, link, fmt, ##__VA_ARGS__) (...) #define ata_dev_dbg(dev, fmt, ...) \ - ata_dev_printk(dev, KERN_DEBUG, fmt, ##__VA_ARGS__) + ata_dev_printk(debug, dev, fmt, ##__VA_ARGS__ So, instead of printk(..., level == KERN_DEBUG, ) we now call pr_debug(...). This is a problem as printk(msg, KERN_DEBUG) != pr_debug(msg). pr_debug is defined as: /* If you are writing a driver, please use dev_dbg instead */ #if defined(CONFIG_DYNAMIC_DEBUG) || \ (defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE)) #include <linux/dynamic_debug.h> /** * pr_debug - Print a debug-level message conditionally * @fmt: format string * @...: arguments for the format string * * This macro expands to dynamic_pr_debug() if CONFIG_DYNAMIC_DEBUG is * set. Otherwise, if DEBUG is defined, it's equivalent to a printk with * KERN_DEBUG loglevel. If DEBUG is not defined it does nothing. * * It uses pr_fmt() to generate the format string (dynamic_pr_debug() uses * pr_fmt() internally). */ #define pr_debug(fmt, ...) \ dynamic_pr_debug(fmt, ##__VA_ARGS__) #elif defined(DEBUG) #define pr_debug(fmt, ...) \ printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) #else #define pr_debug(fmt, ...) \ no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) #endif Without CONFIG_DYNAMIC_DEBUG and if no CONFIG_DEBUG is enabled, the end result is calling no_printk which means nothing gets logged. Looking at the code, there are more impacted calls, like for example ata_dev_dbg(dev, "disabling queued TRIM support\n") for ATA_HORKAGE_NO_NCQ_TRIM, which also seems like an important information to log, and there are more. Was this change done intentionally? Note that most of the "debug" printks in libata code seem to be guarded by ata_msg_info / ata_msg_probe / ATA_DEBUG which was sufficient to prevent excess debug information logging. One of the cases like this was covered in the patch: -#ifdef ATA_DEBUG if (status != 0xff && (status & (ATA_BUSY | ATA_DRQ))) - ata_port_printk(ap, KERN_DEBUG, "abnormal Status 0x%X\n", - status); -#endif + ata_port_dbg(ap, "abnormal Status 0x%X\n", status); Assuming this is the intended direction, would it make sense for now to at promote "unconditionally" logged messages from ata_link_dbg/ata_dev_dbg to ata_link_info/ata_dev_info? Longer term, perhaps we want to revisit ata_msg_info/ata_msg_probe/ATA_DEBUG/ATA_VERBOSE_DEBUG vs ata_dev_printk/ata_link_printk/pr_debug (and maybe also pr_devel), especially that DYNAMIC_DEBUG is available these days... Best regards, Krzysztof Olędzki

1 year, 1 month

4
7
0 0

FAILED: patch "[PATCH] nilfs2: fix use-after-free of timer for log writer thread" failed to apply to 4.19-stable tree

by gregkh＠linuxfoundation.org

The patch below does not apply to the 4.19-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <stable(a)vger.kernel.org>. To reproduce the conflict and resubmit, you may use the following commands: git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-4.19.y git checkout FETCH_HEAD git cherry-pick -x f5d4e04634c9cf68bdf23de08ada0bb92e8befe7 # <resolve conflicts, build, test, etc.> git commit -s git send-email --to '<stable(a)vger.kernel.org>' --in-reply-to '2024052609-speckled-elusive-2d6c@gregkh' --subject-prefix 'PATCH 4.19.y' HEAD^.. Possible dependencies: thanks, greg k-h ------------------ original commit in Linus's tree ------------------ From f5d4e04634c9cf68bdf23de08ada0bb92e8befe7 Mon Sep 17 00:00:00 2001 From: Ryusuke Konishi <konishi.ryusuke(a)gmail.com> Date: Mon, 20 May 2024 22:26:19 +0900 Subject: [PATCH] nilfs2: fix use-after-free of timer for log writer thread Patch series "nilfs2: fix log writer related issues". This bug fix series covers three nilfs2 log writer-related issues, including a timer use-after-free issue and potential deadlock issue on unmount, and a potential freeze issue in event synchronization found during their analysis. Details are described in each commit log. This patch (of 3): A use-after-free issue has been reported regarding the timer sc_timer on the nilfs_sc_info structure. The problem is that even though it is used to wake up a sleeping log writer thread, sc_timer is not shut down until the nilfs_sc_info structure is about to be freed, and is used regardless of the thread's lifetime. Fix this issue by limiting the use of sc_timer only while the log writer thread is alive. Link: https://lkml.kernel.org/r/20240520132621.4054-1-konishi.ryusuke@gmail.com Link: https://lkml.kernel.org/r/20240520132621.4054-2-konishi.ryusuke@gmail.com Fixes: fdce895ea5dd ("nilfs2: change sc_timer from a pointer to an embedded one in struct nilfs_sc_info") Signed-off-by: Ryusuke Konishi <konishi.ryusuke(a)gmail.com> Reported-by: "Bai, Shuangpeng" <sjb7183(a)psu.edu> Closes: https://groups.google.com/g/syzkaller/c/MK_LYqtt8ko/m/8rgdWeseAwAJ Tested-by: Ryusuke Konishi <konishi.ryusuke(a)gmail.com> Cc: <stable(a)vger.kernel.org> Signed-off-by: Andrew Morton <akpm(a)linux-foundation.org> diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c index 6be7dd423fbd..7cb34e1c9206 100644 --- a/fs/nilfs2/segment.c +++ b/fs/nilfs2/segment.c @@ -2118,8 +2118,10 @@ static void nilfs_segctor_start_timer(struct nilfs_sc_info *sci) { spin_lock(&sci->sc_state_lock); if (!(sci->sc_state & NILFS_SEGCTOR_COMMIT)) { - sci->sc_timer.expires = jiffies + sci->sc_interval; - add_timer(&sci->sc_timer); + if (sci->sc_task) { + sci->sc_timer.expires = jiffies + sci->sc_interval; + add_timer(&sci->sc_timer); + } sci->sc_state |= NILFS_SEGCTOR_COMMIT; } spin_unlock(&sci->sc_state_lock); @@ -2320,10 +2322,21 @@ int nilfs_construct_dsync_segment(struct super_block *sb, struct inode *inode, */ static void nilfs_segctor_accept(struct nilfs_sc_info *sci) { + bool thread_is_alive; + spin_lock(&sci->sc_state_lock); sci->sc_seq_accepted = sci->sc_seq_request; + thread_is_alive = (bool)sci->sc_task; spin_unlock(&sci->sc_state_lock); - del_timer_sync(&sci->sc_timer); + + /* + * This function does not race with the log writer thread's + * termination. Therefore, deleting sc_timer, which should not be + * done after the log writer thread exits, can be done safely outside + * the area protected by sc_state_lock. + */ + if (thread_is_alive) + del_timer_sync(&sci->sc_timer); } /** @@ -2349,7 +2362,7 @@ static void nilfs_segctor_notify(struct nilfs_sc_info *sci, int mode, int err) sci->sc_flush_request &= ~FLUSH_DAT_BIT; /* re-enable timer if checkpoint creation was not done */ - if ((sci->sc_state & NILFS_SEGCTOR_COMMIT) && + if ((sci->sc_state & NILFS_SEGCTOR_COMMIT) && sci->sc_task && time_before(jiffies, sci->sc_timer.expires)) add_timer(&sci->sc_timer); } @@ -2539,6 +2552,7 @@ static int nilfs_segctor_thread(void *arg) int timeout = 0; sci->sc_timer_task = current; + timer_setup(&sci->sc_timer, nilfs_construction_timeout, 0); /* start sync. */ sci->sc_task = current; @@ -2606,6 +2620,7 @@ static int nilfs_segctor_thread(void *arg) end_thread: /* end sync. */ sci->sc_task = NULL; + timer_shutdown_sync(&sci->sc_timer); wake_up(&sci->sc_wait_task); /* for nilfs_segctor_kill_thread() */ spin_unlock(&sci->sc_state_lock); return 0; @@ -2669,7 +2684,6 @@ static struct nilfs_sc_info *nilfs_segctor_new(struct super_block *sb, INIT_LIST_HEAD(&sci->sc_gc_inodes); INIT_LIST_HEAD(&sci->sc_iput_queue); INIT_WORK(&sci->sc_iput_work, nilfs_iput_work_func); - timer_setup(&sci->sc_timer, nilfs_construction_timeout, 0); sci->sc_interval = HZ * NILFS_SC_DEFAULT_TIMEOUT; sci->sc_mjcp_freq = HZ * NILFS_SC_DEFAULT_SR_FREQ; @@ -2748,7 +2762,6 @@ static void nilfs_segctor_destroy(struct nilfs_sc_info *sci) down_write(&nilfs->ns_segctor_sem); - timer_shutdown_sync(&sci->sc_timer); kfree(sci); }

1 year, 1 month

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

Linux-stable-mirror May 2024