huggingface / huggingface/candle

potential deadlock risk from multiple read locks over storage in tensor operations

Open
#3,878 3 comments 1 reaction 0 assignees View on GitHub
Dominant language
Rust
Stars
21k
Forks
1.8k
Avg merge
16h 42m
Merged PRs (30d)
25

Description

Multiple tensor operations (for example `binary_op`) take two (or more) read locks over underlying storage that can deadlock if the `self `and `rhs`/`kernel` "other" storages are the same and there is a concurrent write lock request on the same storage. [std-sync RwLock](https://doc.rust-lang.org/std/sync/struct.RwLock.html) can deadlock two reads on the same thread if there is a concurrent write.

Since `storage()` takes a read lock and returns the guard
https://github.com/huggingface/candle/blob/6f74e7c390c717f8fd34f23ce02aceb058173370/candle-core/src/tensor.rs#L2741-L2743
for the following example from `binary_op`
https://github.com/huggingface/candle/blob/6f74e7c390c717f8fd34f23ce02aceb058173370/candle-core/src/tensor.rs#L101-L102
when `self `and `rhs` are the same, the underlying storage and its lock would be the same.

There is a similar reported issue https://github.com/huggingface/candle/issues/3227 that shows deadlock without concurrent writes, these are included here too but are a different kind of bug.
The deadlocks with no concurrency in **custom_op**:
| Function | Guards |
|---|---|
| `Tensor::scatter_set` | `self`[W] → `indexes`[R], `source`[R] |
| `Tensor::scatter_add_set` | `self`[W] → `indexes`[R], `source`[R] |
| `Tensor::inplace_op2` | `self`[W] → `rhs`[R] |
| `Tensor::inplace_op3` | `self`[W] → `t2`[R], `t3`[R] |

`scatter_set` and `scatter_add_set` already guard against source and self being the same, but not against indexes also being the same (seems like the "same storage" check is for correctness rather than deadlock prevention here). `Tensor::slice_set` and `Var::set` already check if they have the same storage and bail if true.

Deadlocks can occur if there are concurrent writes between the `.storage()` calls over the following variables:

In **tensor.rs**:
| Function | Overlapping |
|---|---|
| `add`, `sub`, `mul`, `div` | `self` → `rhs` |
| `cmp` | `self` → `rhs` |
| `matmul` | `self` → `rhs` |
| `where_cond` | `self`, `on_true`, `on_false` |
| `scatter` | `indexes` → `source` |
| `scatter_add` | `indexes` → `source` |
| `index_add` | `self`, `indexes`, `source` |
| `gather` | `self` → `indexes` |
| `index_select` | `self` → `indexes` |

In **conv.rs**, all `self` → `kernel`:
| Function |
|---|
| `conv1d_single_group` |
| `conv_transpose1d_single_group` |
| `conv2d_single_group` |
| `conv_transpose2d` |

In **custom_op.rs**:
| Function | Overlapping |
|---|---|
| `apply_op2_no_bwd` | `self` → `rhs` |
| `apply_op3_no_bwd` | `self`, `t2`, `t3` |
| `apply_op2_arc` | `self` → `rhs` |
| `apply_op3_arc` | `self`, `t2`, `t3` |

These possible deadlocks were found by a static analysis tool. There could be more deadlocks hiding behind `storage_and_layout()`.

The fix should check whether the two storages are the same before attempting to lock the second one or switch over to a reentrant RwLock like the one in parking_lot.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in candle-core/src/tensor.rs with storage(), binary_op, and the listed tensor operations, then inspect the corresponding functions in conv.rs and custom_op.rs. Compare their lock ordering with the existing same-storage checks in Tensor::slice_set and Var::set, and trace storage_and_layout(). Done means the listed overlapping-storage cases no longer risk deadlock, including concurrent-write scenarios.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
machine-learning
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.