gpioc: fix race in ioctl(GPIOCONFIGEVENTS)
ClosedPublic
Actions

Authored by vexeduxr on Mon, Sep 29, 3:47 PM.

Details

Reviewers

mmel
andrew
jhb
markj
manu

Commits

rG4472ecb362b0: gpioc: fix race in ioctl(GPIOCONFIGEVENTS)
rG6163ced966b4: gpioc: fix race in ioctl(GPIOCONFIGEVENTS)
rG8fef0ee89497: gpioc: fix race in ioctl(GPIOCONFIGEVENTS)
rGd000adfe41e6: gpioc: fix race in ioctl(GPIOCONFIGEVENTS)

Summary

A race can occur in gpioc_ioctl when it is called with GPIOCONFIGEVENTS
closely followed by GPIOSETCONFIG. GPIOSETCONFIG can alter the
priv->pins list, making it no longer empty and opening the door for
access to priv->events while we are reallocating it. Fix this by holding
priv->mtx while handling GPIOCONFIGEVENTS.

Reported by: Qiu-ji Chen
PR: 289120
MFC after: 3 days

Test Plan

This race would be quite hard to hit "naturally", but I managed to hit it with a strategically placed DELAY.
As expected, the race is gone when we hold priv->mtx.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 67407
Build 64290: arc lint + arc unit

Event Timeline

vexeduxr created this revision.Mon, Sep 29, 3:47 PM

Herald added subscribers: loos, imp. · View Herald TranscriptMon, Sep 29, 3:47 PM

vexeduxr requested review of this revision.Mon, Sep 29, 3:47 PM

Harbormaster completed remote builds in B67390: Diff 163023.Mon, Sep 29, 3:47 PM

vexeduxr edited the test plan for this revision. (Show Details)Mon, Sep 29, 3:50 PM

malloc priv->events with the correct size

Harbormaster completed remote builds in B67404: Diff 163062.Tue, Sep 30, 6:30 AM

mmel added inline comments.Tue, Sep 30, 6:43 AM

sys/dev/gpio/gpioc.c
938	imho, M_NOWAIT is incorrect in this context. M_NOWAIT allocation can fail at any time, and in a non-deterministic manner. Not only when the system runs out of memory, but also when the other side locks one of the allocator's internal structures.

vexeduxr added inline comments.Tue, Sep 30, 6:53 AM

sys/dev/gpio/gpioc.c
938	We can't use `M_WAITOK` while holding a mutex though. We do the same in `gpioc_attach_priv_pin` when allocating a new list entry.

mmel added inline comments.Tue, Sep 30, 7:23 AM

sys/dev/gpio/gpioc.c
938	malloc() should not be used with a mutex if the code cannot retry in case of failure; this is bad design practice. In this case, I don't see any disadvantage in using SX lock at first glance if malloc() cannot be moved outside. However, this raises another question: if the GPIO is located on serial buses (SPI, I2C), its functions may sleep. Is GPIOC aware of this fact?

mmel added inline comments.Tue, Sep 30, 7:52 AM

sys/dev/gpio/gpioc.c
938	Oups, sorry. Mutex is used in interrupt, so SX lock is out of the question.

vexeduxr added inline comments.Tue, Sep 30, 9:17 AM

sys/dev/gpio/gpioc.c
938	The malloc can be moved outside with some rearchitecting. And giving this some more thought, I agree that the call shouldn't fail if the system is under memory strain.

Allocate fifo before locking priv->mtx

Harbormaster completed remote builds in B67407: Diff 163079.Tue, Sep 30, 9:20 AM

mmel accepted this revision.Tue, Sep 30, 10:13 AM

This revision is now accepted and ready to land.Tue, Sep 30, 10:13 AM

Closed by commit rGd000adfe41e6: gpioc: fix race in ioctl(GPIOCONFIGEVENTS) (authored by vexeduxr). · Explain WhyTue, Sep 30, 11:21 AM

This revision was automatically updated to reflect the committed changes.

vexeduxr added a commit: rGd000adfe41e6: gpioc: fix race in ioctl(GPIOCONFIGEVENTS).

vexeduxr added inline comments.Tue, Sep 30, 9:25 PM

sys/dev/gpio/gpioc.c
938	However, this raises another question: if the GPIO is located on serial buses (SPI, I2C), its functions may sleep. Is GPIOC aware of this fact? Ah, forgot to reply to this. The code doesn't hold any locks when calling the GPIO functions, and it drops it's locks when setting up the interrupt.