* [PATCH 0/3] firewire: use single object for user space listeners to dispatch request to IEC 61883-1 FCP region
@ 2023-01-20 9:03 Takashi Sakamoto
2023-01-20 9:03 ` [PATCH 1/3] firewire: core: use kref structure to maintain lifetime of data for fw_request structure Takashi Sakamoto
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2023-01-20 9:03 UTC (permalink / raw)
To: tiwai, stefanr; +Cc: linux1394-devel, linux-kernel, alsa-devel
Hi,
This patch solves long standing issue mentioned by code comment[1] and a
commit 281e20323ab7 ("firewire: core: fix use-after-free regression in FCP
handler")[2]. This patchset is based on the kernel tree to which another
fix is applied[3].
To Iwai-san, I would like to ask you picking them to your local
tree, then send them to mainline tree as well as sound patches when
the merge window is open for v6.3 kernel, unless any question and
objection is posted. (Additionally, I have prepared the other patchset for
the subsystem.)
Linux FireWire subsystem allows multiple listeners to inbound request to
IEC 61883-1 FCP region. The listeners need to access to the payload of
request, while the core function needs to guarantee transaction completion
by sending response. It is a risk to use-after-free by accessing to the
object of request released at completion.
The commit 281e20323ab7 solves the risk by the duplication of payload,
while it is slightly redundant (and a risk of memory leak[3]). Developers
have acknowledged that the advantage of reference count to maintain the
lifetime of object.
Furthermore, the commit uses NULL pointer for the listeners to
distinguish whether the request is to FCP region or not. It is a risk
that the listeners retrieve parameters by dereferencing it. For
instance, a commit b2405aa948b9 ("firewire: add kernel API to access
packet structure in request structure for AR context")[4] adds a kernel
API to retrieve time stamp from the data of request, while it is not
available in the case of request to FCP region.
As a solution, this patchset uses kref structure to maintain lifetime of
data for fw_request structure, then dismisses NULL pointer passing to the
listeners. Finally, the duplication is obsoleted.
This is a log to probe kmalloc/kfree tracepoints events in the case to
handle the request to FCP region.
irq/35-firewire-273881 [002] 373149.138580: kmalloc: (fw_core_handle_request+0x7b) call_site=fw_core_handle_request+0x7b ptr=0xffff99e93f007300 bytes_req=155 bytes_alloc=192 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 373149.138582: kmalloc: (handle_request+0x22e) call_site=handle_request+0x22e ptr=0xffff99e96e54f240 bytes_req=56 bytes_alloc=64 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 373149.138583: kmalloc: (handle_request+0x24b) call_site=handle_request+0x24b ptr=0xffff99e8b8a6ed20 bytes_req=96 bytes_alloc=96 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 373149.138587: kmalloc: (handle_request+0x22e) call_site=handle_request+0x22e ptr=0xffff99e96e54fa80 bytes_req=56 bytes_alloc=64 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 373149.138587: kmalloc: (handle_request+0x24b) call_site=handle_request+0x24b ptr=0xffff99e8a3f06960 bytes_req=96 bytes_alloc=96 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 373149.138591: kmalloc: (handle_request+0x22e) call_site=handle_request+0x22e ptr=0xffff99e96e54f380 bytes_req=56 bytes_alloc=64 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 373149.138591: kmalloc: (handle_request+0x24b) call_site=handle_request+0x24b ptr=0xffff99e8a3f06600 bytes_req=96 bytes_alloc=96 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 373149.138598: kmalloc: (handle_request+0x22e) call_site=handle_request+0x22e ptr=0xffff99e96e54f7c0 bytes_req=56 bytes_alloc=64 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 373149.138598: kmalloc: (handle_request+0x24b) call_site=handle_request+0x24b ptr=0xffff99e8a3f06180 bytes_req=96 bytes_alloc=96 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 373149.138602: kmalloc: (handle_request+0x22e) call_site=handle_request+0x22e ptr=0xffff99e96e54f4c0 bytes_req=56 bytes_alloc=64 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 373149.138603: kmalloc: (handle_request+0x24b) call_site=handle_request+0x24b ptr=0xffff99e94d48b660 bytes_req=96 bytes_alloc=96 gfp_flags=GFP_ATOMIC
python3-278050 [000] 373149.138605: kfree: (fw_device_op_read+0x148) call_site=fw_device_op_read+0x148 ptr=0xffff99e8b8a6ed20
python3-278050 [000] 373149.138611: kfree: (ioctl_send_response+0x10a) call_site=ioctl_send_response+0x10a ptr=0xffff99e96e54f240
python3-278053 [001] 373149.138612: kfree: (fw_device_op_read+0x148) call_site=fw_device_op_read+0x148 ptr=0xffff99e8a3f06960
snd-oxfw-ctl-se-278061 [003] 373149.138618: kfree: (fw_device_op_read+0x148) call_site=fw_device_op_read+0x148 ptr=0xffff99e94d48b660
python3-278053 [001] 373149.138621: kfree: (ioctl_send_response+0x10a) call_site=ioctl_send_response+0x10a ptr=0xffff99e96e54fa80
python3-278057 [002] 373149.138627: kfree: (fw_device_op_read+0x148) call_site=fw_device_op_read+0x148 ptr=0xffff99e8a3f06180
snd-oxfw-ctl-se-278061 [003] 373149.138636: kfree: (ioctl_send_response+0x10a) call_site=ioctl_send_response+0x10a ptr=0xffff99e96e54f4c0
python3-278055 [000] 373149.138637: kfree: (fw_device_op_read+0x148) call_site=fw_device_op_read+0x148 ptr=0xffff99e8a3f06600
python3-278055 [000] 373149.138642: kfree: (ioctl_send_response+0x10a) call_site=ioctl_send_response+0x10a ptr=0xffff99e96e54f380
python3-278057 [002] 373149.138654: kfree: (fw_request_put+0x2c) call_site=fw_request_put+0x2c ptr=0xffff99e93f007300
python3-278057 [002] 373149.138654: kfree: (ioctl_send_response+0x10a) call_site=ioctl_send_response+0x10a ptr=0xffff99e96e54f7c0
There are five listeners of IEC 61883-1 FCP region:
* Python 3: PID 278050
* Python 3: PID 278053
* Python 3: PID 278055
* Python 3: PID 278057
* snd-oxfw-ctl-service: PID 278061
When handling 1394 OHCI Asynchronous Receive context (for request) in
softIRQ context, an object is allocated for the request
(ptr=0xffff99e93f007300). When queueing event per client, the reference
count of object is increased.
The object is released when the last listener (PID 278057) executes
ioctl(2) with FW_CDEV_IOC_SEND_REQUEST request to decrease the reference
count and release event resource.
Between the event queueing and event dispatching, the core function send
response to complete transaction. The object is not released in the timing,
expectedly.
This is another log in the case to handle request to non-FCP region:
irq/35-firewire-273881 [002] 377484.348477: kmalloc: (fw_core_handle_request+0x7b) call_site=fw_core_handle_request+0x7b ptr=0xffff99e96140cd80 bytes_req=148 bytes_alloc=192 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 377484.348478: kmalloc: (handle_request+0xa8) call_site=handle_request+0xa8 ptr=0xffff99e9669303c0 bytes_req=56 bytes_alloc=64 gfp_flags=GFP_ATOMIC
irq/35-firewire-273881 [002] 377484.348479: kmalloc: (handle_request+0xc4) call_site=handle_request+0xc4 ptr=0xffff99e99eaed0c0 bytes_req=96 bytes_alloc=96 gfp_flags=GFP_ATOMIC
python3-280425 [001] 377484.348494: kfree: (fw_device_op_read+0x148) call_site=fw_device_op_read+0x148 ptr=0xffff99e99eaed0c0
python3-280425 [001] 377484.348563: kfree: (ioctl_send_response+0x10a) call_site=ioctl_send_response+0x10a ptr=0xffff99e9669303c0
irq/35-firewire-273881 [002] 377484.348603: kfree: (free_response_callback+0x5a) call_site=free_response_callback+0x5a ptr=0xffff99e96140cd80
The object is allocated (ptr=0xffff99e96140cd80) in the softIRQ, then
event is queued. Python 3 process (PID 280425) dispatches the event, then
execute ioctl(2) to send response. In softIRQ for 1394 OHCI Asynchronous
Receive context (for response), the object is finally released as a result
to decrease the reference count.
The reference count looks to prevent memory leak.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firewire/core-cdev.c?h=v6.1#n691
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=281e20323ab72180137824a298ee9e21e6f9acf6
[3] https://lore.kernel.org/lkml/20230117090610.93792-1-o-takashi@sakamocchi.jp/
[4] I should have elaborated the patch title...
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b2405aa948b95afc5246fa56fc05c3512cd6185c
Takashi Sakamoto (3):
firewire: core: use kref structure to maintain lifetime of data for
fw_request structure
firewire: cdev: obsolete NULL check to detect IEC 61883-1 FCP region
firewire: cdev: use single object to dispatch event for request to IEC
61883-1 FCP region
drivers/firewire/core-cdev.c | 43 +++++++++--------------
drivers/firewire/core-transaction.c | 53 +++++++++++++++++++++--------
drivers/firewire/core.h | 9 +++++
include/linux/firewire.h | 5 ++-
4 files changed, 67 insertions(+), 43 deletions(-)
--
2.37.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] firewire: core: use kref structure to maintain lifetime of data for fw_request structure
2023-01-20 9:03 [PATCH 0/3] firewire: use single object for user space listeners to dispatch request to IEC 61883-1 FCP region Takashi Sakamoto
@ 2023-01-20 9:03 ` Takashi Sakamoto
2023-01-20 9:03 ` [PATCH 2/3] firewire: cdev: obsolete NULL check to detect IEC 61883-1 FCP region Takashi Sakamoto
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2023-01-20 9:03 UTC (permalink / raw)
To: tiwai, stefanr; +Cc: linux1394-devel, linux-kernel, alsa-devel
Developers have acknowledged that maintenance of lifetime for
fw_transaction structure is effective when handling asynchronous
transaction to IEC 61883-1 FCP region, since the core function allows
multiples listeners to the region. Some of them needs to access to the
payload of request in process context after the callback to listener,
while the core function releases the object for the structure just after
completing the callbacks to listeners.
One of the listeners is character device. Current implementation of the
character device duplicates the object for the payload of transaction,
while it's a cost in kernel memory consumption. The lifetime management
can reduce it.
The typical way to maintain the lifetime is reference count. This commit
uses kref structure as a first step for the purpose.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-cdev.c | 4 ++--
drivers/firewire/core-transaction.c | 33 +++++++++++++++++++++++++----
drivers/firewire/core.h | 3 +++
3 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 958aa4662ccb..93dd80d8d4e1 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -826,12 +826,12 @@ static int ioctl_send_response(struct client *client, union ioctl_arg *arg)
if (a->length != fw_get_response_length(r->request)) {
ret = -EINVAL;
- kfree(r->request);
+ fw_request_put(r->request);
goto out;
}
if (copy_from_user(r->data, u64_to_uptr(a->data), a->length)) {
ret = -EFAULT;
- kfree(r->request);
+ fw_request_put(r->request);
goto out;
}
fw_send_response(r->card, r->request, a->rcode);
diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index af498d767702..83f61cf1aa8f 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -617,6 +617,7 @@ void fw_core_remove_address_handler(struct fw_address_handler *handler)
EXPORT_SYMBOL(fw_core_remove_address_handler);
struct fw_request {
+ struct kref kref;
struct fw_packet response;
u32 request_header[4];
int ack;
@@ -625,13 +626,33 @@ struct fw_request {
u32 data[];
};
+void fw_request_get(struct fw_request *request)
+{
+ kref_get(&request->kref);
+}
+
+static void release_request(struct kref *kref)
+{
+ struct fw_request *request = container_of(kref, struct fw_request, kref);
+
+ kfree(request);
+}
+
+void fw_request_put(struct fw_request *request)
+{
+ kref_put(&request->kref, release_request);
+}
+
static void free_response_callback(struct fw_packet *packet,
struct fw_card *card, int status)
{
- struct fw_request *request;
+ struct fw_request *request = container_of(packet, struct fw_request, response);
- request = container_of(packet, struct fw_request, response);
- kfree(request);
+ // Decrease the reference count since not at in-flight.
+ fw_request_put(request);
+
+ // Decrease the reference count to release the object.
+ fw_request_put(request);
}
int fw_get_response_length(struct fw_request *r)
@@ -782,6 +803,7 @@ static struct fw_request *allocate_request(struct fw_card *card,
request = kmalloc(sizeof(*request) + length, GFP_ATOMIC);
if (request == NULL)
return NULL;
+ kref_init(&request->kref);
request->response.speed = p->speed;
request->response.timestamp =
@@ -809,7 +831,7 @@ void fw_send_response(struct fw_card *card,
/* unified transaction or broadcast transaction: don't respond */
if (request->ack != ACK_PENDING ||
HEADER_DESTINATION_IS_BROADCAST(request->request_header[0])) {
- kfree(request);
+ fw_request_put(request);
return;
}
@@ -821,6 +843,9 @@ void fw_send_response(struct fw_card *card,
fw_fill_response(&request->response, request->request_header,
rcode, NULL, 0);
+ // Increase the reference count so that the object is kept during in-flight.
+ fw_request_get(request);
+
card->driver->send_response(card, &request->response);
}
EXPORT_SYMBOL(fw_send_response);
diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h
index 71d5f16f311c..78c99f1d27fa 100644
--- a/drivers/firewire/core.h
+++ b/drivers/firewire/core.h
@@ -244,6 +244,9 @@ int fw_get_response_length(struct fw_request *request);
void fw_fill_response(struct fw_packet *response, u32 *request_header,
int rcode, void *payload, size_t length);
+void fw_request_get(struct fw_request *request);
+void fw_request_put(struct fw_request *request);
+
#define FW_PHY_CONFIG_NO_NODE_ID -1
#define FW_PHY_CONFIG_CURRENT_GAP_COUNT -1
void fw_send_phy_config(struct fw_card *card,
--
2.37.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] firewire: cdev: obsolete NULL check to detect IEC 61883-1 FCP region
2023-01-20 9:03 [PATCH 0/3] firewire: use single object for user space listeners to dispatch request to IEC 61883-1 FCP region Takashi Sakamoto
2023-01-20 9:03 ` [PATCH 1/3] firewire: core: use kref structure to maintain lifetime of data for fw_request structure Takashi Sakamoto
@ 2023-01-20 9:03 ` Takashi Sakamoto
2023-01-20 9:03 ` [PATCH 3/3] firewire: cdev: use single object to dispatch event for request to " Takashi Sakamoto
2023-01-23 8:22 ` [PATCH 0/3] firewire: use single object for user space listeners to dispatch " Takashi Iwai
3 siblings, 0 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2023-01-20 9:03 UTC (permalink / raw)
To: tiwai, stefanr; +Cc: linux1394-devel, linux-kernel, alsa-devel
In the character device, the listener to address space should distinguish
whether the request is to IEC 61883-1 FCP region or not. The user space
application needs to access to the object of request in enough later by
read(2), while the core function releases the object of request in the FCP
case after completing the callback to handler.
The handler guarantees the access safe by some way. It's done by
duplication of the object after NULL check to the request, since core
function passes NULL in the FCP case. It's inconvenient since the object
of request includes some helpful information. It's better to add another
way to check whether the request is to FCP region or not.
Conveniently the file of transaction layer includes local implementation
for the purpose. This commit moves it to module local file and use it
instead of the NULL check, then the result of check is stored to
per-client data for the inbound transaction so that the result can be
referred by later to release the data.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-cdev.c | 16 +++++++---------
drivers/firewire/core-transaction.c | 20 ++++++++++----------
drivers/firewire/core.h | 6 ++++++
include/linux/firewire.h | 5 ++---
4 files changed, 25 insertions(+), 22 deletions(-)
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 93dd80d8d4e1..7fa49e51bae8 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -111,6 +111,7 @@ struct inbound_transaction_resource {
struct client_resource resource;
struct fw_card *card;
struct fw_request *request;
+ bool is_fcp;
void *data;
size_t length;
};
@@ -643,18 +644,13 @@ static int ioctl_send_request(struct client *client, union ioctl_arg *arg)
client->device->max_speed);
}
-static inline bool is_fcp_request(struct fw_request *request)
-{
- return request == NULL;
-}
-
static void release_request(struct client *client,
struct client_resource *resource)
{
struct inbound_transaction_resource *r = container_of(resource,
struct inbound_transaction_resource, resource);
- if (is_fcp_request(r->request))
+ if (r->is_fcp)
kfree(r->data);
else
fw_send_response(r->card, r->request, RCODE_CONFLICT_ERROR);
@@ -669,6 +665,7 @@ static void handle_request(struct fw_card *card, struct fw_request *request,
void *payload, size_t length, void *callback_data)
{
struct address_handler_resource *handler = callback_data;
+ bool is_fcp = is_in_fcp_region(offset, length);
struct inbound_transaction_resource *r;
struct inbound_transaction_event *e;
size_t event_size0;
@@ -685,10 +682,11 @@ static void handle_request(struct fw_card *card, struct fw_request *request,
r->card = card;
r->request = request;
+ r->is_fcp = is_fcp;
r->data = payload;
r->length = length;
- if (is_fcp_request(request)) {
+ if (is_fcp) {
/*
* FIXME: Let core-transaction.c manage a
* single reference-counted copy?
@@ -743,7 +741,7 @@ static void handle_request(struct fw_card *card, struct fw_request *request,
kfree(e);
kfree(fcp_frame);
- if (!is_fcp_request(request))
+ if (!is_fcp)
fw_send_response(card, request, RCODE_CONFLICT_ERROR);
fw_card_put(card);
@@ -819,7 +817,7 @@ static int ioctl_send_response(struct client *client, union ioctl_arg *arg)
r = container_of(resource, struct inbound_transaction_resource,
resource);
- if (is_fcp_request(r->request)) {
+ if (r->is_fcp) {
kfree(r->data);
goto out;
}
diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 83f61cf1aa8f..a9f70c96323e 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -535,12 +535,6 @@ const struct fw_address_region fw_unit_space_region =
{ .start = 0xfffff0000900ULL, .end = 0x1000000000000ULL, };
#endif /* 0 */
-static bool is_in_fcp_region(u64 offset, size_t length)
-{
- return offset >= (CSR_REGISTER_BASE | CSR_FCP_COMMAND) &&
- offset + length <= (CSR_REGISTER_BASE | CSR_FCP_END);
-}
-
/**
* fw_core_add_address_handler() - register for incoming requests
* @handler: callback
@@ -822,12 +816,18 @@ static struct fw_request *allocate_request(struct fw_card *card,
return request;
}
+/**
+ * fw_send_response: - send response packet for asynchronous transaction.
+ * @card: interface to send the response at.
+ * @request: firewire request data for the transaction.
+ * @rcode: response code to send.
+ *
+ * Submit a response packet into the asynchronous response transmission queue. The @request
+ * is going to be released when the transmission successfully finishes later.
+ */
void fw_send_response(struct fw_card *card,
struct fw_request *request, int rcode)
{
- if (WARN_ONCE(!request, "invalid for FCP address handlers"))
- return;
-
/* unified transaction or broadcast transaction: don't respond */
if (request->ack != ACK_PENDING ||
HEADER_DESTINATION_IS_BROADCAST(request->request_header[0])) {
@@ -935,7 +935,7 @@ static void handle_fcp_region_request(struct fw_card *card,
rcu_read_lock();
list_for_each_entry_rcu(handler, &address_handler_list, link) {
if (is_enclosing_handler(handler, offset, request->length))
- handler->address_callback(card, NULL, tcode,
+ handler->address_callback(card, request, tcode,
destination, source,
p->generation, offset,
request->data,
diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h
index 78c99f1d27fa..eafa4eaae737 100644
--- a/drivers/firewire/core.h
+++ b/drivers/firewire/core.h
@@ -257,4 +257,10 @@ static inline bool is_ping_packet(u32 *data)
return (data[0] & 0xc0ffffff) == 0 && ~data[0] == data[1];
}
+static inline bool is_in_fcp_region(u64 offset, size_t length)
+{
+ return offset >= (CSR_REGISTER_BASE | CSR_FCP_COMMAND) &&
+ offset + length <= (CSR_REGISTER_BASE | CSR_FCP_END);
+}
+
#endif /* _FIREWIRE_CORE_H */
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 980019053e54..56505436d159 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -278,9 +278,8 @@ typedef void (*fw_transaction_callback_t)(struct fw_card *card, int rcode,
* Otherwise there is a danger of recursion of inbound and outbound
* transactions from and to the local node.
*
- * The callback is responsible that either fw_send_response() or kfree()
- * is called on the @request, except for FCP registers for which the core
- * takes care of that.
+ * The callback is responsible that fw_send_response() is called on the @request, except for FCP
+ * registers for which the core takes care of that.
*/
typedef void (*fw_address_callback_t)(struct fw_card *card,
struct fw_request *request,
--
2.37.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] firewire: cdev: use single object to dispatch event for request to IEC 61883-1 FCP region
2023-01-20 9:03 [PATCH 0/3] firewire: use single object for user space listeners to dispatch request to IEC 61883-1 FCP region Takashi Sakamoto
2023-01-20 9:03 ` [PATCH 1/3] firewire: core: use kref structure to maintain lifetime of data for fw_request structure Takashi Sakamoto
2023-01-20 9:03 ` [PATCH 2/3] firewire: cdev: obsolete NULL check to detect IEC 61883-1 FCP region Takashi Sakamoto
@ 2023-01-20 9:03 ` Takashi Sakamoto
2023-01-23 8:22 ` [PATCH 0/3] firewire: use single object for user space listeners to dispatch " Takashi Iwai
3 siblings, 0 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2023-01-20 9:03 UTC (permalink / raw)
To: tiwai, stefanr; +Cc: linux1394-devel, linux-kernel, alsa-devel
The core function always passes the data of request to the callback of
listener in any case. Additionally, the listener can maintain the lifetime
of data by reference count. In character device, no need to duplicate the
payload of request anymore to copy it to user space.
This commit extends the lifetime of data to obsolete duplication of
payload for request in character device.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-cdev.c | 25 +++++++++----------------
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 7fa49e51bae8..2c16ee8fd842 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -651,7 +651,7 @@ static void release_request(struct client *client,
struct inbound_transaction_resource, resource);
if (r->is_fcp)
- kfree(r->data);
+ fw_request_put(r->request);
else
fw_send_response(r->card, r->request, RCODE_CONFLICT_ERROR);
@@ -669,12 +669,16 @@ static void handle_request(struct fw_card *card, struct fw_request *request,
struct inbound_transaction_resource *r;
struct inbound_transaction_event *e;
size_t event_size0;
- void *fcp_frame = NULL;
int ret;
/* card may be different from handler->client->device->card */
fw_card_get(card);
+ // Extend the lifetime of data for request so that its payload is safely accessible in
+ // the process context for the client.
+ if (is_fcp)
+ fw_request_get(request);
+
r = kmalloc(sizeof(*r), GFP_ATOMIC);
e = kmalloc(sizeof(*e), GFP_ATOMIC);
if (r == NULL || e == NULL)
@@ -686,18 +690,6 @@ static void handle_request(struct fw_card *card, struct fw_request *request,
r->data = payload;
r->length = length;
- if (is_fcp) {
- /*
- * FIXME: Let core-transaction.c manage a
- * single reference-counted copy?
- */
- fcp_frame = kmemdup(payload, length, GFP_ATOMIC);
- if (fcp_frame == NULL)
- goto failed;
-
- r->data = fcp_frame;
- }
-
r->resource.release = release_request;
ret = add_client_resource(handler->client, &r->resource, GFP_ATOMIC);
if (ret < 0)
@@ -739,10 +731,11 @@ static void handle_request(struct fw_card *card, struct fw_request *request,
failed:
kfree(r);
kfree(e);
- kfree(fcp_frame);
if (!is_fcp)
fw_send_response(card, request, RCODE_CONFLICT_ERROR);
+ else
+ fw_request_put(request);
fw_card_put(card);
}
@@ -818,7 +811,7 @@ static int ioctl_send_response(struct client *client, union ioctl_arg *arg)
r = container_of(resource, struct inbound_transaction_resource,
resource);
if (r->is_fcp) {
- kfree(r->data);
+ fw_request_put(r->request);
goto out;
}
--
2.37.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] firewire: use single object for user space listeners to dispatch request to IEC 61883-1 FCP region
2023-01-20 9:03 [PATCH 0/3] firewire: use single object for user space listeners to dispatch request to IEC 61883-1 FCP region Takashi Sakamoto
` (2 preceding siblings ...)
2023-01-20 9:03 ` [PATCH 3/3] firewire: cdev: use single object to dispatch event for request to " Takashi Sakamoto
@ 2023-01-23 8:22 ` Takashi Iwai
2023-01-25 12:07 ` Takashi Sakamoto
3 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2023-01-23 8:22 UTC (permalink / raw)
To: Takashi Sakamoto; +Cc: stefanr, linux1394-devel, linux-kernel, alsa-devel
On Fri, 20 Jan 2023 10:03:41 +0100,
Takashi Sakamoto wrote:
>
> Hi,
>
> This patch solves long standing issue mentioned by code comment[1] and a
> commit 281e20323ab7 ("firewire: core: fix use-after-free regression in FCP
> handler")[2]. This patchset is based on the kernel tree to which another
> fix is applied[3].
>
> To Iwai-san, I would like to ask you picking them to your local
> tree, then send them to mainline tree as well as sound patches when
> the merge window is open for v6.3 kernel, unless any question and
> objection is posted. (Additionally, I have prepared the other patchset for
> the subsystem.)
As those are spontaneous small fixes, now I merged all three patches
on topic/firewire branch (on top of the for-linus including your
previous FireWire core fix), merged back to for-next branch for 6.3.
But, I have no will to keep doing this in a long term. I suppose the
best would be that you'd step up as a maintainer for FireWire
stack...
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] firewire: use single object for user space listeners to dispatch request to IEC 61883-1 FCP region
2023-01-23 8:22 ` [PATCH 0/3] firewire: use single object for user space listeners to dispatch " Takashi Iwai
@ 2023-01-25 12:07 ` Takashi Sakamoto
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2023-01-25 12:07 UTC (permalink / raw)
To: Takashi Iwai; +Cc: stefanr, linux1394-devel, linux-kernel, alsa-devel
Hi,
On Mon, Jan 23, 2023 at 09:22:51AM +0100, Takashi Iwai wrote:
> On Fri, 20 Jan 2023 10:03:41 +0100,
> Takashi Sakamoto wrote:
> >
> > Hi,
> >
> > This patch solves long standing issue mentioned by code comment[1] and a
> > commit 281e20323ab7 ("firewire: core: fix use-after-free regression in FCP
> > handler")[2]. This patchset is based on the kernel tree to which another
> > fix is applied[3].
> >
> > To Iwai-san, I would like to ask you picking them to your local
> > tree, then send them to mainline tree as well as sound patches when
> > the merge window is open for v6.3 kernel, unless any question and
> > objection is posted. (Additionally, I have prepared the other patchset for
> > the subsystem.)
>
> As those are spontaneous small fixes, now I merged all three patches
> on topic/firewire branch (on top of the for-linus including your
> previous FireWire core fix), merged back to for-next branch for 6.3.
Thanks for your applying.
> But, I have no will to keep doing this in a long term. I suppose the
> best would be that you'd step up as a maintainer for FireWire
> stack...
Indeed. The next patchset is beyond your courtesy. I posted it to LKML
with my concern. I'm pleased if you follow to it.
* https://lore.kernel.org/lkml/20230125120301.51585-1-o-takashi@sakamocchi.jp/
> thanks,
>
> Takashi
Thanks
Takashi Sakamoto
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-25 12:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-20 9:03 [PATCH 0/3] firewire: use single object for user space listeners to dispatch request to IEC 61883-1 FCP region Takashi Sakamoto
2023-01-20 9:03 ` [PATCH 1/3] firewire: core: use kref structure to maintain lifetime of data for fw_request structure Takashi Sakamoto
2023-01-20 9:03 ` [PATCH 2/3] firewire: cdev: obsolete NULL check to detect IEC 61883-1 FCP region Takashi Sakamoto
2023-01-20 9:03 ` [PATCH 3/3] firewire: cdev: use single object to dispatch event for request to " Takashi Sakamoto
2023-01-23 8:22 ` [PATCH 0/3] firewire: use single object for user space listeners to dispatch " Takashi Iwai
2023-01-25 12:07 ` Takashi Sakamoto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox