by Bobby Eshleman

[permalink] [raw]

Subject: Re: [PATCH RFC net-next v3 1/8] vsock/dgram: generalize recvmsg and drop transport->dgram_dequeue

On Wed, May 31, 2023 at 05:56:48PM +0200, Simon Horman wrote:
> On Wed, May 31, 2023 at 12:35:05AM +0000, Bobby Eshleman wrote:
> > This commit drops the transport->dgram_dequeue callback and makes
> > vsock_dgram_recvmsg() generic. It also adds additional transport
> > callbacks for use by the generic vsock_dgram_recvmsg(), such as for
> > parsing skbs for CID/port which vary in format per transport.
> >
> > Signed-off-by: Bobby Eshleman <[email protected]>
>
> ...
>
> > diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> > index b370070194fa..b6a51afb74b8 100644
> > --- a/net/vmw_vsock/vmci_transport.c
> > +++ b/net/vmw_vsock/vmci_transport.c
> > @@ -1731,57 +1731,40 @@ static int vmci_transport_dgram_enqueue(
> > return err - sizeof(*dg);
> > }
> >
> > -static int vmci_transport_dgram_dequeue(struct vsock_sock *vsk,
> > - struct msghdr *msg, size_t len,
> > - int flags)
> > +int vmci_transport_dgram_get_cid(struct sk_buff *skb, unsigned int *cid)
> > {
> > - int err;
> > struct vmci_datagram *dg;
> > - size_t payload_len;
> > - struct sk_buff *skb;
> >
> > - if (flags & MSG_OOB || flags & MSG_ERRQUEUE)
> > - return -EOPNOTSUPP;
> > + dg = (struct vmci_datagram *)skb->data;
> > + if (!dg)
> > + return -EINVAL;
> >
> > - /* Retrieve the head sk_buff from the socket's receive queue. */
> > - err = 0;
> > - skb = skb_recv_datagram(&vsk->sk, flags, &err);
> > - if (!skb)
> > - return err;
> > + *cid = dg->src.context;
> > + return 0;
> > +}
>
> Hi Bobby,
>
> clang-16 with W=1 seems a bit unhappy about this.
>
> net/vmw_vsock/vmci_transport.c:1734:5: warning: no previous prototype for function 'vmci_transport_dgram_get_cid' [-Wmissing-prototypes]
> int vmci_transport_dgram_get_cid(struct sk_buff *skb, unsigned int *cid)
> ^
> net/vmw_vsock/vmci_transport.c:1734:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
> int vmci_transport_dgram_get_cid(struct sk_buff *skb, unsigned int *cid)
> ^
> static
> net/vmw_vsock/vmci_transport.c:1746:5: warning: no previous prototype for function 'vmci_transport_dgram_get_port' [-Wmissing-prototypes]
> int vmci_transport_dgram_get_port(struct sk_buff *skb, unsigned int *port)
> ^
> net/vmw_vsock/vmci_transport.c:1746:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
> int vmci_transport_dgram_get_port(struct sk_buff *skb, unsigned int *port)
> ^
> static
> net/vmw_vsock/vmci_transport.c:1758:5: warning: no previous prototype for function 'vmci_transport_dgram_get_length' [-Wmissing-prototypes]
> int vmci_transport_dgram_get_length(struct sk_buff *skb, size_t *len)
> ^
> net/vmw_vsock/vmci_transport.c:1758:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
> int vmci_transport_dgram_get_length(struct sk_buff *skb, size_t *len)
> ^
>
> I see similar warnings for net/vmw_vsock/af_vsock.c in patch 4/8.
>
> > +
> > +int vmci_transport_dgram_get_port(struct sk_buff *skb, unsigned int *port)
> > +{
> > + struct vmci_datagram *dg;
> >
> > dg = (struct vmci_datagram *)skb->data;
> > if (!dg)
> > - /* err is 0, meaning we read zero bytes. */
> > - goto out;
> > -
> > - payload_len = dg->payload_size;
> > - /* Ensure the sk_buff matches the payload size claimed in the packet. */
> > - if (payload_len != skb->len - sizeof(*dg)) {
> > - err = -EINVAL;
> > - goto out;
> > - }
> > + return -EINVAL;
> >
> > - if (payload_len > len) {
> > - payload_len = len;
> > - msg->msg_flags |= MSG_TRUNC;
> > - }
> > + *port = dg->src.resource;
> > + return 0;
> > +}
> >
> > - /* Place the datagram payload in the user's iovec. */
> > - err = skb_copy_datagram_msg(skb, sizeof(*dg), msg, payload_len);
> > - if (err)
> > - goto out;
> > +int vmci_transport_dgram_get_length(struct sk_buff *skb, size_t *len)
> > +{
> > + struct vmci_datagram *dg;
> >
> > - if (msg->msg_name) {
> > - /* Provide the address of the sender. */
> > - DECLARE_SOCKADDR(struct sockaddr_vm *, vm_addr, msg->msg_name);
> > - vsock_addr_init(vm_addr, dg->src.context, dg->src.resource);
> > - msg->msg_namelen = sizeof(*vm_addr);
> > - }
> > - err = payload_len;
> > + dg = (struct vmci_datagram *)skb->data;
> > + if (!dg)
> > + return -EINVAL;
> >
> > -out:
> > - skb_free_datagram(&vsk->sk, skb);
> > - return err;
> > + *len = dg->payload_size;
> > + return 0;
> > }
> >
> > static bool vmci_transport_dgram_allow(u32 cid, u32 port)
>
> ...

Thanks for the review! Your feedback from both emails will be
incorporated in the next rev (with C=1 and W=1 output clearing).

Thanks again,
Bobby

2023-06-07 17:14:37

by Bobby Eshleman

[permalink] [raw]

Subject: Re: [PATCH RFC net-next v3 6/8] virtio/vsock: support dgrams

On Wed, May 31, 2023 at 09:13:04PM +0300, Dan Carpenter wrote:
> On Wed, May 31, 2023 at 06:09:11PM +0200, Simon Horman wrote:
> > > @@ -102,6 +144,7 @@ virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *info,
> >
> > Smatch that err may not be initialised in the out label below.
> >
> > Just above this context the following appears:
> >
> > if (info->vsk && !skb_set_owner_sk_safe(skb, sk_vsock(info->vsk))) {
> > WARN_ONCE(1, "failed to allocate skb on vsock socket with sk_refcnt == 0\n");
> > goto out;
> > }
> >
> > So I wonder if in that case err may not be initialised.
> >
>
> Yep, exactly right. I commented out the goto and it silenced the
> warning. I also initialized err to zero at the start hoping that it
> would trigger a different warning but it didn't. :(
>
> regards,
> dan carpenter
>

Thanks for checking that Dan. Fixed in the next rev.

Best,
Bobby