netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Taehee Yoo <ap420073@gmail.com>
To: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com,
	edumazet@google.com, netdev@vger.kernel.org
Cc: ap420073@gmail.com
Subject: [PATCH net 0/8] amt: fix validation and synchronization bugs
Date: Tue, 12 Jul 2022 10:57:06 +0000	[thread overview]
Message-ID: <20220712105714.12282-1-ap420073@gmail.com> (raw)

There are some synchronization issues in the amt module.
Especially, an amt gateway doesn't well synchronize its own variables
and status(amt->status).
It tries to use a workqueue for handles in a single thread.
A global lock is also good, but it would occur complex locking complex.

In this patchset, only the gateway uses workqueue.
The reason why only gateway interface uses workqueue is that gateway
should manage its own states and variables a little bit statefully.
But relay doesn't need to manage tunnels statefully, stateless is okay.
So, relay side message handlers are okay to be called concurrently.
But it doesn't mean that no lock is needed.

Only amt multicast data message type will not be processed by the work
queue because It contains actual multicast data.
So, it should be processed immediately.

When any amt gateway events are triggered(sending discovery message by
delayed_work, sending request message by delayed_work and receiving
messages), it stores event and skb into the event queue(amt->events[16]).
Then, workqueue processes these events one by one.

The first patch is to use the work queue.

The second patch is to remove unnecessary lock due to a previous patch.

The third patch is to use READ_ONCE() in the amt module.
Even if the amt module uses a single thread, some variables (ready4,
ready6, amt->status) can be accessed concurrently.

The fourth patch is to add missing nonce generation logic when it sends a
new request message.

The fifth patch is to drop unexpected advertisement messages.
advertisement message should be received only after the gateway sends
a discovery message first.
So, the gateway should drop advertisement messages if it has never
sent a discovery message and it also should drop duplicate advertisement
messages.
Using nonce is good to distinguish whether a received message is an
expected message or not.

The sixth patch is to drop unexpected query messages.
This is the same behavior as the fourth patch.
Query messages should be received only after the gateway sends a request
message first.
The nonce variable is used to distinguish whether it is a reply to a
previous request message or not.
amt->ready4 and amt->ready6 are used to distinguish duplicate messages.

The seventh patch is to drop unexpected multicast data.
AMT gateway should not receive multicast data message type before
establish between gateway and relay.
In order to drop unexpected multicast data messages, it checks amt->status.

The last patch is to fix a locking problem on the relay side.
amt->nr_tunnels variable is protected by amt->lock.
But amt_request_handler() doesn't protect this variable.

Taehee Yoo (8):
  amt: use workqueue for gateway side message handling
  amt: remove unnecessary locks
  amt: use READ_ONCE() in amt module
  amt: add missing regeneration nonce logic in request logic
  amt: drop unexpected advertisement message
  amt: drop unexpected query message
  amt: drop unexpected multicast data
  amt: do not use amt->nr_tunnels outside of lock

 drivers/net/amt.c | 238 ++++++++++++++++++++++++++++++++++++----------
 include/net/amt.h |  20 ++++
 2 files changed, 206 insertions(+), 52 deletions(-)

-- 
2.17.1


             reply	other threads:[~2022-07-12 10:57 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-12 10:57 Taehee Yoo [this message]
2022-07-12 10:57 ` [PATCH net 1/8] amt: use workqueue for gateway side message handling Taehee Yoo
2022-07-14  3:55   ` Jakub Kicinski
2022-07-15 12:34     ` Taehee Yoo
2022-07-14  8:09   ` Paolo Abeni
2022-07-15 12:51     ` Taehee Yoo
2022-07-12 10:57 ` [PATCH net 2/8] amt: remove unnecessary locks Taehee Yoo
2022-07-12 10:57 ` [PATCH net 3/8] amt: use READ_ONCE() in amt module Taehee Yoo
2022-07-12 10:57 ` [PATCH net 4/8] amt: add missing regeneration nonce logic in request logic Taehee Yoo
2022-07-12 10:57 ` [PATCH net 5/8] amt: drop unexpected advertisement message Taehee Yoo
2022-07-12 10:57 ` [PATCH net 6/8] amt: drop unexpected query message Taehee Yoo
2022-07-12 10:57 ` [PATCH net 7/8] amt: drop unexpected multicast data Taehee Yoo
2022-07-12 10:57 ` [PATCH net 8/8] amt: do not use amt->nr_tunnels outside of lock Taehee Yoo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220712105714.12282-1-ap420073@gmail.com \
    --to=ap420073@gmail.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).