| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <11efcbb2-f69a-49b4-a593-34fce42d49ea@icloud.com>
Date: Sun, 18 Aug 2024 22:24:30 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, Davidlohr Bueso
<dave@...olabs.net>, Jonathan Cameron <jonathan.cameron@...wei.com>,
Dave Jiang <dave.jiang@...el.com>,
Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
Dan Williams <dan.j.williams@...el.com>, Timur Tabi <timur@...nel.org>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org,
linux1394-devel@...ts.sourceforge.net, netdev@...r.kernel.org,
Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH v2 3/4] firewire: core: Prevent device_find_child() from
modifying caller's match data
On 2024/8/17 17:57, Takashi Sakamoto wrote:
> Hi,
>
> On Thu, Aug 15, 2024 at 10:58:04PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@...cinc.com>
>>
>> To prepare for constifying the following old driver core API:
>>
>> struct device *device_find_child(struct device *dev, void *data,
>> int (*match)(struct device *dev, void *data));
>> to new:
>> struct device *device_find_child(struct device *dev, const void *data,
>> int (*match)(struct device *dev, const void *data));
>>
>> The new API does not allow its match function (*match)() to modify
>> caller's match data @*data, but lookup_existing_device() as the old
>> API's match function indeed modifies relevant match data, so it is not
>> suitable for the new API any more, fixed by implementing a equivalent
>> fw_device_find_child() instead of the old API usage.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
>> ---
>> drivers/firewire/core-device.c | 37 +++++++++++++++++++++++++++++++++++--
>> 1 file changed, 35 insertions(+), 2 deletions(-)
>
> Thanks for the patch.
>
>> Why to constify the API ?
>>
>> (1) It normally does not make sense, also does not need to, for
>> such device finding operation to modify caller's match data which
>> is mainly used for comparison.
>>
>> (2) It will make the API's match function and match data parameter
>> have the same type as all other APIs (bus|class|driver)_find_device().
>>
>> (3) It will give driver author hints about choice between this API and
>> the following one:
>> int device_for_each_child(struct device *dev, void *data,
>> int (*fn)(struct device *dev, void *data));
>
> I have found another issue in respect to this subsystem.
>
> The whole procedure in 'lookup_existing_device()' in the call of
> 'device_find_child()' is a bit superfluous, since it includes not only
> finding but also updating. The helper function passed to
> 'device_find_child()' should do quick finding only.
>
> I think we can change the relevant codes like the following patch. It
> would solve your concern, too. If you prefer the change, I'm going to
> evaluate it.
>
thanks for your reply.
of course, i prefer your change.
> ======== 8< --------
>
> From ceaa8a986ae07865eb3fec810de330e96b6d56e2 Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi@...amocchi.jp>
> Date: Sat, 17 Aug 2024 17:52:53 +0900
> Subject: [PATCH] firewire: core: update fw_device outside of
> device_find_child()
>
> When detecting updates of bus topology, the data of fw_device is newly
> allocated and caches the content of configuration ROM from the
> corresponding node. Then, the tree of device is sought to find the
> previous data of fw_device corresponding to the node, since in IEEE 1394
> specification numeric node identifier could be changed dynamically every
> generation of bus topology. If it is found, the previous data is updated
> and reused, then the newly allocated data is going to be released.
>
> The above procedure is done in the call of device_find_child(), however it
> is a bit abusing against the intention of the helper function, since the
> call would not only find but also update.
>
> This commit splits the update outside of the call.
> ---
> drivers/firewire/core-device.c | 109 ++++++++++++++++-----------------
> 1 file changed, 54 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
> index bc4c9e5a..62e8d839 100644
> --- a/drivers/firewire/core-device.c
> +++ b/drivers/firewire/core-device.c
> @@ -928,56 +928,6 @@ static void fw_device_update(struct work_struct *work)
> device_for_each_child(&device->device, NULL, update_unit);
> }
>
> -/*
> - * If a device was pending for deletion because its node went away but its
> - * bus info block and root directory header matches that of a newly discovered
> - * device, revive the existing fw_device.
> - * The newly allocated fw_device becomes obsolete instead.
> - */
> -static int lookup_existing_device(struct device *dev, void *data)
> -{
> - struct fw_device *old = fw_device(dev);
> - struct fw_device *new = data;
> - struct fw_card *card = new->card;
> - int match = 0;
> -
> - if (!is_fw_device(dev))
> - return 0;
> -
> - guard(rwsem_read)(&fw_device_rwsem); // serialize config_rom access
> - guard(spinlock_irq)(&card->lock); // serialize node access
> -
> - if (memcmp(old->config_rom, new->config_rom, 6 * 4) == 0 &&
> - atomic_cmpxchg(&old->state,
> - FW_DEVICE_GONE,
> - FW_DEVICE_RUNNING) == FW_DEVICE_GONE) {
> - struct fw_node *current_node = new->node;
> - struct fw_node *obsolete_node = old->node;
> -
> - new->node = obsolete_node;
> - new->node->data = new;
> - old->node = current_node;
> - old->node->data = old;
> -
> - old->max_speed = new->max_speed;
> - old->node_id = current_node->node_id;
> - smp_wmb(); /* update node_id before generation */
> - old->generation = card->generation;
> - old->config_rom_retries = 0;
> - fw_notice(card, "rediscovered device %s\n", dev_name(dev));
> -
> - old->workfn = fw_device_update;
> - fw_schedule_device_work(old, 0);
> -
> - if (current_node == card->root_node)
> - fw_schedule_bm_work(card, 0);
> -
> - match = 1;
> - }
> -
> - return match;
> -}
> -
> enum { BC_UNKNOWN = 0, BC_UNIMPLEMENTED, BC_IMPLEMENTED, };
>
> static void set_broadcast_channel(struct fw_device *device, int generation)
> @@ -1038,6 +988,17 @@ int fw_device_set_broadcast_channel(struct device *dev, void *gen)
> return 0;
> }
>
> +static int compare_configuration_rom(struct device *dev, void *data)
> +{
> + const struct fw_device *old = fw_device(dev);
> + const u32 *config_rom = data;
> +
> + if (!is_fw_device(dev))
> + return 0;
> +
> + return !!memcmp(old->config_rom, config_rom, 6 * 4);
!memcmp(old->config_rom, config_rom, 6 * 4) ?
is this extra condition old->state == FW_DEVICE_GONE required ?
namely, is it okay for below return ?
return !memcmp(old->config_rom, config_rom, 6 * 4) && old->state ==
FW_DEVICE_GONE
> +}
> +
> static void fw_device_init(struct work_struct *work)
> {
> struct fw_device *device =
> @@ -1071,13 +1032,51 @@ static void fw_device_init(struct work_struct *work)
> return;
> }
>
> - revived_dev = device_find_child(card->device,
> - device, lookup_existing_device);
> + // If a device was pending for deletion because its node went away but its bus info block
> + // and root directory header matches that of a newly discovered device, revive the
> + // existing fw_device. The newly allocated fw_device becomes obsolete instead.
> + //
> + // serialize config_rom access.
> + scoped_guard(rwsem_read, &fw_device_rwsem) {
> + // TODO: The cast to 'void *' could be removed if Zijun Hu's work goes well.
may remove this TODO line since i will simply remove the cast with the
other patch series as shown below:
https://lore.kernel.org/all/20240811-const_dfc_done-v1-0-9d85e3f943cb@quicinc.com/
> + revived_dev = device_find_child(card->device, (void *)device->config_rom,
> + compare_configuration_rom);
> + }
> if (revived_dev) {
> - put_device(revived_dev);
> - fw_device_release(&device->device);
> + struct fw_device *found = fw_device(revived_dev);
>
> - return;
> + // serialize node access
> + guard(spinlock_irq)(&card->lock);
> +
> + if (atomic_cmpxchg(&found->state,
> + FW_DEVICE_GONE,
> + FW_DEVICE_RUNNING) == FW_DEVICE_GONE) {
> + struct fw_node *current_node = device->node;
> + struct fw_node *obsolete_node = found->node;
> +
> + device->node = obsolete_node;
> + device->node->data = device;
> + found->node = current_node;
> + found->node->data = found;
> +
> + found->max_speed = device->max_speed;
> + found->node_id = current_node->node_id;
> + smp_wmb(); /* update node_id before generation */
> + found->generation = card->generation;
> + found->config_rom_retries = 0;
> + fw_notice(card, "rediscovered device %s\n", dev_name(revived_dev));
> +
> + found->workfn = fw_device_update;
> + fw_schedule_device_work(found, 0);
> +
> + if (current_node == card->root_node)
> + fw_schedule_bm_work(card, 0);
> +
> + put_device(revived_dev);
> + fw_device_release(&device->device);
> +
> + return;
> + }
is it okay to put_device() here as well ?
put_device(revived_dev);
> }
>
> device_initialize(&device->device);
Powered by blists - more mailing lists