| Message ID | AM6PR03MB5848595A20BB5D958C2D9DE299272@AM6PR03MB5848.eurprd03.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net/packet: Add getsockopt support for PACKET_COPY_THRESH | expand |
Juntong Deng wrote: > Currently getsockopt does not support PACKET_COPY_THRESH, > and we are unable to get the value of PACKET_COPY_THRESH > socket option through getsockopt. > > This patch adds getsockopt support for PACKET_COPY_THRESH. > > Signed-off-by: Juntong Deng <juntong.deng@outlook.com> > --- > net/packet/af_packet.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 0db31ca4982d..65042edd1a97 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -4090,6 +4090,9 @@ static int packet_getsockopt(struct socket *sock, int level, int optname, > case PACKET_VNET_HDR_SZ: > val = READ_ONCE(po->vnet_hdr_sz); > break; > + case PACKET_COPY_THRESH: > + val = pkt_sk(sk)->copy_thresh; > + break; This is probably a good opportunity to use READ_ONCE/WRITE_ONCE for this variable that can be modified and read concurrently. Alternatively I can fix up all three locations in a follow-on patch. No concerns with adding the getsockopt itself. > case PACKET_VERSION: > val = po->tp_version; > break; > -- > 2.39.2 >
On 2024/3/8 19:43, Willem de Bruijn wrote: > Juntong Deng wrote: >> Currently getsockopt does not support PACKET_COPY_THRESH, >> and we are unable to get the value of PACKET_COPY_THRESH >> socket option through getsockopt. >> >> This patch adds getsockopt support for PACKET_COPY_THRESH. >> >> Signed-off-by: Juntong Deng <juntong.deng@outlook.com> >> --- >> net/packet/af_packet.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c >> index 0db31ca4982d..65042edd1a97 100644 >> --- a/net/packet/af_packet.c >> +++ b/net/packet/af_packet.c >> @@ -4090,6 +4090,9 @@ static int packet_getsockopt(struct socket *sock, int level, int optname, >> case PACKET_VNET_HDR_SZ: >> val = READ_ONCE(po->vnet_hdr_sz); >> break; >> + case PACKET_COPY_THRESH: >> + val = pkt_sk(sk)->copy_thresh; >> + break; > > This is probably a good opportunity to use READ_ONCE/WRITE_ONCE for > this variable that can be modified and read concurrently. > > Alternatively I can fix up all three locations in a follow-on patch. > > No concerns with adding the getsockopt itself. > >> case PACKET_VERSION: >> val = po->tp_version; >> break; >> -- >> 2.39.2 >> > > Thanks for reviewing. So, do I need to send PATCH V2 to add READ_ONCE? Or do you want you to use a follow-on patch to fix all three locations at once?
Juntong Deng wrote: > On 2024/3/8 19:43, Willem de Bruijn wrote: > > Juntong Deng wrote: > >> Currently getsockopt does not support PACKET_COPY_THRESH, > >> and we are unable to get the value of PACKET_COPY_THRESH > >> socket option through getsockopt. > >> > >> This patch adds getsockopt support for PACKET_COPY_THRESH. > >> > >> Signed-off-by: Juntong Deng <juntong.deng@outlook.com> > >> --- > >> net/packet/af_packet.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > >> index 0db31ca4982d..65042edd1a97 100644 > >> --- a/net/packet/af_packet.c > >> +++ b/net/packet/af_packet.c > >> @@ -4090,6 +4090,9 @@ static int packet_getsockopt(struct socket *sock, int level, int optname, > >> case PACKET_VNET_HDR_SZ: > >> val = READ_ONCE(po->vnet_hdr_sz); > >> break; > >> + case PACKET_COPY_THRESH: > >> + val = pkt_sk(sk)->copy_thresh; > >> + break; > > > > This is probably a good opportunity to use READ_ONCE/WRITE_ONCE for > > this variable that can be modified and read concurrently. > > > > Alternatively I can fix up all three locations in a follow-on patch. > > > > No concerns with adding the getsockopt itself. > > > >> case PACKET_VERSION: > >> val = po->tp_version; > >> break; > >> -- > >> 2.39.2 > >> > > > > > > Thanks for reviewing. > > So, do I need to send PATCH V2 to add READ_ONCE? > > Or do you want you to use a follow-on patch to fix all three locations > at once? Please use READ_ONCE and convert the existing accesses to copy_thresh in the same patch to READ_ONCE/WRITE_ONCE. That's simplest.
On Fri, Mar 8, 2024 at 7:43 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Juntong Deng wrote: > > Currently getsockopt does not support PACKET_COPY_THRESH, > > and we are unable to get the value of PACKET_COPY_THRESH > > socket option through getsockopt. > > > > This patch adds getsockopt support for PACKET_COPY_THRESH. > > > > Signed-off-by: Juntong Deng <juntong.deng@outlook.com> > > --- > > net/packet/af_packet.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > > index 0db31ca4982d..65042edd1a97 100644 > > --- a/net/packet/af_packet.c > > +++ b/net/packet/af_packet.c > > @@ -4090,6 +4090,9 @@ static int packet_getsockopt(struct socket *sock, int level, int optname, > > case PACKET_VNET_HDR_SZ: > > val = READ_ONCE(po->vnet_hdr_sz); > > break; > > + case PACKET_COPY_THRESH: > > + val = pkt_sk(sk)->copy_thresh; > > + break; > > This is probably a good opportunity to use READ_ONCE/WRITE_ONCE for > this variable that can be modified and read concurrently. > > Alternatively I can fix up all three locations in a follow-on patch. Yes, variables which can be set through setsockopt related functions have a chance to be modified when another process does read it. The patch you mentioned goes like this, I think: diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 0db31ca4982d..b598dcafe441 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -2318,7 +2318,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, } if (po->tp_version <= TPACKET_V2) { if (macoff + snaplen > po->rx_ring.frame_size) { - if (po->copy_thresh && + if (READ_ONCE(po->copy_thresh) && atomic_read(&sk->sk_rmem_alloc) < sk->sk_rcvbuf) { if (skb_shared(skb)) { copy_skb = skb_clone(skb, GFP_ATOMIC); @@ -3836,7 +3836,7 @@ packet_setsockopt(struct socket *sock, int level, int optname, sockptr_t optval, if (copy_from_sockptr(&val, optval, sizeof(val))) return -EFAULT; - pkt_sk(sk)->copy_thresh = val; + WRITE_ONCE(pkt_sk(sk)->copy_thresh, val); return 0; } case PACKET_VERSION: diff --git a/net/packet/diag.c b/net/packet/diag.c index b3bd2f6c2bf7..47f69f3dbf73 100644 --- a/net/packet/diag.c +++ b/net/packet/diag.c @@ -17,7 +17,7 @@ static int pdiag_put_info(const struct packet_sock *po, struct sk_buff *nlskb) pinfo.pdi_index = po->ifindex; pinfo.pdi_version = po->tp_version; pinfo.pdi_reserve = po->tp_reserve; - pinfo.pdi_copy_thresh = po->copy_thresh; + pinfo.pdi_copy_thresh = READ_ONCE(po->copy_thresh); pinfo.pdi_tstamp = READ_ONCE(po->tp_tstamp); pinfo.pdi_flags = 0; Thanks, Jason > > No concerns with adding the getsockopt itself. > > > case PACKET_VERSION: > > val = po->tp_version; > > break; > > -- > > 2.39.2 > > > > >
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 0db31ca4982d..65042edd1a97 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -4090,6 +4090,9 @@ static int packet_getsockopt(struct socket *sock, int level, int optname, case PACKET_VNET_HDR_SZ: val = READ_ONCE(po->vnet_hdr_sz); break; + case PACKET_COPY_THRESH: + val = pkt_sk(sk)->copy_thresh; + break; case PACKET_VERSION: val = po->tp_version; break;
Currently getsockopt does not support PACKET_COPY_THRESH, and we are unable to get the value of PACKET_COPY_THRESH socket option through getsockopt. This patch adds getsockopt support for PACKET_COPY_THRESH. Signed-off-by: Juntong Deng <juntong.deng@outlook.com> --- net/packet/af_packet.c | 3 +++ 1 file changed, 3 insertions(+)