On Fri, Oct 17, 2025 at 1:15 PM Ilya Maximets <[email protected]> wrote:
> In a few places the list sizes are checked without holding the lock.
> While this probably speeds up the processing a bit, it's technically
> incorrect as the access is not atomic. Caught by thread-safety
> analyzer of clang 21, as it now properly supports GUARDED annotations
> on structure fields:
>
> lib/ipf.c:1117:33:
> error: reading the value pointed to by 'frag_exp_list' requires
> holding any mutex [-Werror,-Wthread-safety-analysis]
> 1117 | if (ovs_list_is_empty(&ipf->frag_exp_list)) {
> | ^
> lib/ipf.c:1154:33:
> error: reading the value pointed to by 'reassembled_pkt_list'
> requires holding any mutex [-Werror,-Wthread-safety-analysis]
> 1154 | if (ovs_list_is_empty(&ipf->reassembled_pkt_list)) {
> | ^
>
> There is also a potential deadlock between the thread destroying the
> ipf and the clean thread as they are using the latch and the ipf lock
> in the opposite order. The latch itself is thread-safe, so we should
> not be holding the lock while setting it and waiting for the clean
> thread that may be waiting for the lock.
>
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
> lib/ipf.c | 33 +++++++++++----------------------
> 1 file changed, 11 insertions(+), 22 deletions(-)
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index b16797312..6ce144bfb 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -1071,12 +1071,9 @@ ipf_send_completed_frags(struct ipf *ipf, struct
> dp_packet_batch *pb,
> long long now, bool v6, uint16_t zone,
> odp_port_t in_port)
> {
> - if (ovs_list_is_empty(&ipf->frag_complete_list)) {
> - return;
> - }
>
I worry about the performance impact with these changes, though I haven't
tested it. I could run a test if you haven't. Receiving a single fragmented
packet on any port would result in all CT threads fighting over a global
lock. Maybe there should be a new atomic counter for nfrags_completed? I
was also thinking about a trylock however the risk with that is not all
threads can handle all completed frags.
> + struct ipf_list *ipf_list;
>
> ovs_mutex_lock(&ipf->ipf_lock);
> - struct ipf_list *ipf_list;
>
> LIST_FOR_EACH_SAFE (ipf_list, list_node, &ipf->frag_complete_list) {
>
> @@ -1114,14 +1111,11 @@ ipf_delete_expired_frags(struct ipf *ipf, long
> long now)
> };
>
>
> - if (ovs_list_is_empty(&ipf->frag_exp_list)) {
> - return;
> - }
> -
> - ovs_mutex_lock(&ipf->ipf_lock);
> struct ipf_list *ipf_list;
> size_t lists_removed = 0;
>
> + ovs_mutex_lock(&ipf->ipf_lock);
> +
> LIST_FOR_EACH_SAFE (ipf_list, list_node, &ipf->frag_exp_list) {
> if (now <= ipf_list->expiration ||
> lists_removed >= IPF_FRAG_LIST_MAX_EXPIRED) {
> @@ -1151,12 +1145,9 @@ static void
> ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb,
> ovs_be16 dl_type)
> {
> - if (ovs_list_is_empty(&ipf->reassembled_pkt_list)) {
> - return;
> - }
> + struct reassembled_pkt *rp;
>
> ovs_mutex_lock(&ipf->ipf_lock);
> - struct reassembled_pkt *rp;
>
> LIST_FOR_EACH_SAFE (rp, rp_list_node, &ipf->reassembled_pkt_list) {
> if (!rp->list->reass_execute_ctx &&
> @@ -1175,12 +1166,9 @@ static void
> ipf_post_execute_reass_pkts(struct ipf *ipf,
> struct dp_packet_batch *pb, bool v6)
> {
> - if (ovs_list_is_empty(&ipf->reassembled_pkt_list)) {
> - return;
> - }
> + struct reassembled_pkt *rp;
>
> ovs_mutex_lock(&ipf->ipf_lock);
> - struct reassembled_pkt *rp;
>
> LIST_FOR_EACH_SAFE (rp, rp_list_node, &ipf->reassembled_pkt_list) {
> const size_t pb_cnt = dp_packet_batch_size(pb);
> @@ -1308,11 +1296,11 @@ ipf_clean_thread_main(void *f)
>
> long long now = time_msec();
>
> + ovs_mutex_lock(&ipf->ipf_lock);
> +
> if (!ovs_list_is_empty(&ipf->frag_exp_list) ||
> !ovs_list_is_empty(&ipf->frag_complete_list)) {
>
Couldn't this conditional just be removed?
-M
>
> - ovs_mutex_lock(&ipf->ipf_lock);
> -
> struct ipf_list *ipf_list;
> LIST_FOR_EACH_SAFE (ipf_list, list_node,
> &ipf->frag_exp_list) {
> @@ -1327,10 +1315,10 @@ ipf_clean_thread_main(void *f)
> ipf_completed_list_clean(&ipf->frag_lists, ipf_list);
> }
> }
> -
> - ovs_mutex_unlock(&ipf->ipf_lock);
> }
>
> + ovs_mutex_unlock(&ipf->ipf_lock);
> +
> poll_timer_wait_until(now + IPF_FRAG_LIST_CLEAN_TIMEOUT);
> latch_wait(&ipf->ipf_clean_thread_exit);
> poll_block();
> @@ -1374,11 +1362,12 @@ ipf_init(void)
> void
> ipf_destroy(struct ipf *ipf)
> {
> - ovs_mutex_lock(&ipf->ipf_lock);
> latch_set(&ipf->ipf_clean_thread_exit);
> pthread_join(ipf->ipf_clean_thread, NULL);
> latch_destroy(&ipf->ipf_clean_thread_exit);
>
> + ovs_mutex_lock(&ipf->ipf_lock);
> +
> struct ipf_list *ipf_list;
> HMAP_FOR_EACH_POP (ipf_list, node, &ipf->frag_lists) {
> while (ipf_list->last_sent_idx < ipf_list->last_inuse_idx) {
> --
> 2.51.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
