| 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
| ||
|
Date: Wed, 27 Mar 2024 10:10:00 +0800
From: Heng Qi <hengqi@...ux.alibaba.com>
To: Dan Jurgens <danielj@...dia.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc: "mst@...hat.com" <mst@...hat.com>,
"jasowang@...hat.com" <jasowang@...hat.com>,
"xuanzhuo@...ux.alibaba.com" <xuanzhuo@...ux.alibaba.com>,
"virtualization@...ts.linux.dev" <virtualization@...ts.linux.dev>,
"davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com"
<pabeni@...hat.com>, Jiri Pirko <jiri@...dia.com>
Subject: Re: [PATCH net-next 4/4] virtio_net: Remove rtnl lock protection of
command buffers
在 2024/3/26 下午11:18, Dan Jurgens 写道:
>> From: Heng Qi <hengqi@...ux.alibaba.com>
>> Sent: Tuesday, March 26, 2024 3:55 AM
>> To: Dan Jurgens <danielj@...dia.com>; netdev@...r.kernel.org
>> Cc: mst@...hat.com; jasowang@...hat.com; xuanzhuo@...ux.alibaba.com;
>> virtualization@...ts.linux.dev; davem@...emloft.net;
>> edumazet@...gle.com; kuba@...nel.org; pabeni@...hat.com; Jiri Pirko
>> <jiri@...dia.com>
>> Subject: Re: [PATCH net-next 4/4] virtio_net: Remove rtnl lock protection of
>> command buffers
>>
>>
>>
>> 在 2024/3/26 上午5:49, Daniel Jurgens 写道:
>>> The rtnl lock is no longer needed to protect the control buffer and
>>> command VQ.
>>>
>>> Signed-off-by: Daniel Jurgens <danielj@...dia.com>
>>> Reviewed-by: Jiri Pirko <jiri@...dia.com>
>>> ---
>>> drivers/net/virtio_net.c | 27 +++++----------------------
>>> 1 file changed, 5 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
>>> 41f8dc16ff38..d09ea20b16be 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -2639,14 +2639,12 @@ static void virtnet_stats(struct net_device
>>> *dev,
>>>
>>> static void virtnet_ack_link_announce(struct virtnet_info *vi)
>>> {
>>> - rtnl_lock();
>>> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
>>> VIRTIO_NET_CTRL_ANNOUNCE_ACK,
>> NULL))
>>> dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
>>> - rtnl_unlock();
>>> }
>>>
>>> -static int _virtnet_set_queues(struct virtnet_info *vi, u16
>>> queue_pairs)
>>> +static int virtnet_set_queues(struct virtnet_info *vi, u16
>>> +queue_pairs)
>>> {
>>> struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
>>> struct scatterlist sg;
>>> @@ -2677,16 +2675,6 @@ static int _virtnet_set_queues(struct
>> virtnet_info *vi, u16 queue_pairs)
>>> return 0;
>>> }
>>>
>>> -static int virtnet_set_queues(struct virtnet_info *vi, u16
>>> queue_pairs) -{
>>> - int err;
>>> -
>>> - rtnl_lock();
>>> - err = _virtnet_set_queues(vi, queue_pairs);
>>> - rtnl_unlock();
>>> - return err;
>>> -}
>>> -
>>> static int virtnet_close(struct net_device *dev)
>>> {
>>> struct virtnet_info *vi = netdev_priv(dev); @@ -3268,7 +3256,7 @@
>>> static int virtnet_set_channels(struct net_device *dev,
>>> return -EINVAL;
>>>
>>> cpus_read_lock();
>>> - err = _virtnet_set_queues(vi, queue_pairs);
>>> + err = virtnet_set_queues(vi, queue_pairs);
>>> if (err) {
>>> cpus_read_unlock();
>>> goto err;
>>> @@ -3558,14 +3546,11 @@ static void virtnet_rx_dim_work(struct
>> work_struct *work)
>>> struct dim_cq_moder update_moder;
>>> int i, qnum, err;
>>>
>>> - if (!rtnl_trylock())
>>> - return;
>>> -
>> Does this guarantee that the synchronization is completely correct?
>>
>> The purpose of this patch set is to add a separate lock for ctrlq rather than
>> reusing the RTNL lock.
>> But for dim workers, it not only involves the use of ctrlq, but also involves
>> reading shared variables in interfaces such as .set_coalesce, .get_coalesce,
>> etc.
> It looks like there is a risk of a dirty read in the get (usecs updated, but not max_packets).
Also dim_enabled.
And later I need to asynchronousize the dim cmds, which means that
different dim workers will operate a shared linked list.
So we need a lock.
> In the set it will return -EINVAL if trying to adjust the settings aside from DIM enabled. I can add a lock for this if you think it's needed, but it doesn't seem like a major problem for debug info.
Not just for debug info, but future extensions as well.
These desynchronizations can introduce more trouble in the future.
Regards,
Heng
>
>
>> In addition, assuming there are 10 queues, each queue is scheduled with its
>> own dim worker at the same time, then these 10 workers may issue
>> parameters to rxq0 10 times in parallel, just because the RTNL lock is
>> removed here.
>>
>> Therefore, when the RTNL lock is removed, a 'for loop' is no longer needed in
>> virtnet_rx_dim_work, and the dim worker of each queue only configures its
>> own parameters.
>>
> Good point. I'll add a new patch to remove the for loop.
>
>> Alternatively, please keep RTNL lock here.
>>
>> Regards,
>> Heng
>>
>>> /* Each rxq's work is queued by "net_dim()->schedule_work()"
>>> * in response to NAPI traffic changes. Note that dim->profile_ix
>>> * for each rxq is updated prior to the queuing action.
>>> * So we only need to traverse and update profiles for all rxqs
>>> - * in the work which is holding rtnl_lock.
>>> + * in the work.
>>> */
>>> for (i = 0; i < vi->curr_queue_pairs; i++) {
>>> rq = &vi->rq[i];
>>> @@ -3587,8 +3572,6 @@ static void virtnet_rx_dim_work(struct
>> work_struct *work)
>>> dim->state = DIM_START_MEASURE;
>>> }
>>> }
>>> -
>>> - rtnl_unlock();
>>> }
>>>
>>> static int virtnet_coal_params_supported(struct ethtool_coalesce
>>> *ec) @@ -4036,7 +4019,7 @@ static int virtnet_xdp_set(struct net_device
>> *dev, struct bpf_prog *prog,
>>> synchronize_net();
>>> }
>>>
>>> - err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>>> + err = virtnet_set_queues(vi, curr_qp + xdp_qp);
>>> if (err)
>>> goto err;
>>> netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); @@ -
>> 4852,7
>>> +4835,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>
>>> virtio_device_ready(vdev);
>>>
>>> - _virtnet_set_queues(vi, vi->curr_queue_pairs);
>>> + virtnet_set_queues(vi, vi->curr_queue_pairs);
>>>
>>> /* a random MAC address has been assigned, notify the device.
>>> * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not
>> there
Powered by blists - more mailing lists