[net] netlink: Don't reorder loads/stores before marking mmap netlink frame as available

Each mmap Netlink frame contains a status field which indicates
whether the frame is unused, reserved, contains data or needs to
be skipped. Both loads and stores may not be reordeded and must
complete before the status field is changed and another CPU might
pick up the frame for use. Use an smp_mb() to cover needs of both
types of callers to netlink_set_status(), callers which have been
reading data frame from the frame, and callers which have been
filling or releasing and thus writing to the frame.

- Example code path requiring a smp_rmb():
  memcpy(skb->data, (void *)hdr + NL_MMAP_HDRLEN, hdr->nm_len);
  netlink_set_status(hdr, NL_MMAP_STATUS_UNUSED);

- Example code path requiring a smp_wmb():
  hdr->nm_uid	= from_kuid(sk_user_ns(sk), NETLINK_CB(skb).creds.uid);
  hdr->nm_gid	= from_kgid(sk_user_ns(sk), NETLINK_CB(skb).creds.gid);
  netlink_frame_flush_dcache(hdr);
  netlink_set_status(hdr, NL_MMAP_STATUS_VALID);

Fixes: f9c228 ("netlink: implement memory mapped recvmsg()")
Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/netlink/af_netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Thomas Graf <tgraf@suug.ch>
Date: Thu, 18 Dec 2014 10:30:26 +0000

> Each mmap Netlink frame contains a status field which indicates
> whether the frame is unused, reserved, contains data or needs to
> be skipped. Both loads and stores may not be reordeded and must
> complete before the status field is changed and another CPU might
> pick up the frame for use. Use an smp_mb() to cover needs of both
> types of callers to netlink_set_status(), callers which have been
> reading data frame from the frame, and callers which have been
> filling or releasing and thus writing to the frame.
> 
> - Example code path requiring a smp_rmb():
>   memcpy(skb->data, (void *)hdr + NL_MMAP_HDRLEN, hdr->nm_len);
>   netlink_set_status(hdr, NL_MMAP_STATUS_UNUSED);
> 
> - Example code path requiring a smp_wmb():
>   hdr->nm_uid	= from_kuid(sk_user_ns(sk), NETLINK_CB(skb).creds.uid);
>   hdr->nm_gid	= from_kgid(sk_user_ns(sk), NETLINK_CB(skb).creds.gid);
>   netlink_frame_flush_dcache(hdr);
>   netlink_set_status(hdr, NL_MMAP_STATUS_VALID);
> 
> Fixes: f9c228 ("netlink: implement memory mapped recvmsg()")
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>

Also applied and queued up for -stable, thanks Thomas.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Dec 18, 2014 at 2:30 AM, Thomas Graf <tgraf@suug.ch> wrote:
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index ef5f77b..2662821 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -550,9 +550,9 @@ static enum nl_mmap_status netlink_get_status(const struct nl_mmap_hdr *hdr)
>  static void netlink_set_status(struct nl_mmap_hdr *hdr,
>                                enum nl_mmap_status status)
>  {
> +       smp_mb();
>         hdr->nm_status = status;
>         flush_dcache_page(pgvec_to_page(hdr));
> -       smp_wmb();
>  }

If this is performance-critical code (I have no idea), then a better
idea might be to use

        smp_store_release(&hdr->nm_status, status);

instead of using explicit memory barriers. That's going to  generally
be much cheaper than a memory barrier.

                      Linus
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html