Re: [PATCH net-next 1/5] virtio_net: Fix an unsafe reference to the page chain
On Sun, May 28, 2023 at 2:29 PM Michael S. Tsirkin wrote: > > On Fri, May 26, 2023 at 02:38:54PM +0800, Jason Wang wrote: > > On Fri, May 26, 2023 at 1:46 PM Liang Chen > > wrote: > > > > > > "private" of buffer page is currently used for big mode to chain pages. > > > But in mergeable mode, that offset of page could mean something else, > > > e.g. when page_pool page is used instead. So excluding mergeable mode to > > > avoid such a problem. > > > > If this issue happens only in the case of page_pool, it would be > > better to squash it there. > > > > Thanks > > > This is a tiny patch so I don't care. Generally it's ok > to first rework code then change functionality. > in this case what Jason says os right especially because > you then do not need to explain that current code is ok. > Sure. it will be squashed into the page pool enablement patch. Thanks! > > > > > > Signed-off-by: Liang Chen > > > --- > > > drivers/net/virtio_net.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 5a7f7a76b920..c5dca0d92e64 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -497,7 +497,7 @@ static struct sk_buff *page_to_skb(struct > > > virtnet_info *vi, > > > return NULL; > > > > > > page = (struct page *)page->private; > > > - if (page) > > > + if (!vi->mergeable_rx_bufs && page) > > > give_pages(rq, page); > > > goto ok; > > > } > > > -- > > > 2.31.1 > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 1/5] virtio_net: Fix an unsafe reference to the page chain
On Sun, May 28, 2023 at 2:16 PM Michael S. Tsirkin wrote: > > On Fri, May 26, 2023 at 01:46:17PM +0800, Liang Chen wrote: > > "private" of buffer page is currently used for big mode to chain pages. > > But in mergeable mode, that offset of page could mean something else, > > e.g. when page_pool page is used instead. So excluding mergeable mode to > > avoid such a problem. > > > > Signed-off-by: Liang Chen > > Ugh the subject makes it looks like current code has a problem > but I don't think so because I don't think anything besides > big packets uses page->private. > > The reason patch is needed is because follow up patches > use page_pool. > pls adjust commit log and subject to make all this clear. > > > > --- > > drivers/net/virtio_net.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 5a7f7a76b920..c5dca0d92e64 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -497,7 +497,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info > > *vi, > > return NULL; > > > > page = (struct page *)page->private; > > - if (page) > > + if (!vi->mergeable_rx_bufs && page) > > To be safe let's limit to big packets too: > > if (!vi->mergeable_rx_bufs && vi->big_packets && page) > > > Sure, thanks! > > give_pages(rq, page); > > goto ok; > > } > > -- > > 2.31.1 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 1/5] virtio_net: Fix an unsafe reference to the page chain
On Fri, May 26, 2023 at 02:38:54PM +0800, Jason Wang wrote: > On Fri, May 26, 2023 at 1:46 PM Liang Chen wrote: > > > > "private" of buffer page is currently used for big mode to chain pages. > > But in mergeable mode, that offset of page could mean something else, > > e.g. when page_pool page is used instead. So excluding mergeable mode to > > avoid such a problem. > > If this issue happens only in the case of page_pool, it would be > better to squash it there. > > Thanks This is a tiny patch so I don't care. Generally it's ok to first rework code then change functionality. in this case what Jason says os right especially because you then do not need to explain that current code is ok. > > > > Signed-off-by: Liang Chen > > --- > > drivers/net/virtio_net.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 5a7f7a76b920..c5dca0d92e64 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -497,7 +497,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info > > *vi, > > return NULL; > > > > page = (struct page *)page->private; > > - if (page) > > + if (!vi->mergeable_rx_bufs && page) > > give_pages(rq, page); > > goto ok; > > } > > -- > > 2.31.1 > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 1/5] virtio_net: Fix an unsafe reference to the page chain
On Fri, May 26, 2023 at 01:46:17PM +0800, Liang Chen wrote: > "private" of buffer page is currently used for big mode to chain pages. > But in mergeable mode, that offset of page could mean something else, > e.g. when page_pool page is used instead. So excluding mergeable mode to > avoid such a problem. > > Signed-off-by: Liang Chen Ugh the subject makes it looks like current code has a problem but I don't think so because I don't think anything besides big packets uses page->private. The reason patch is needed is because follow up patches use page_pool. pls adjust commit log and subject to make all this clear. > --- > drivers/net/virtio_net.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 5a7f7a76b920..c5dca0d92e64 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -497,7 +497,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info > *vi, > return NULL; > > page = (struct page *)page->private; > - if (page) > + if (!vi->mergeable_rx_bufs && page) To be safe let's limit to big packets too: if (!vi->mergeable_rx_bufs && vi->big_packets && page) > give_pages(rq, page); > goto ok; > } > -- > 2.31.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 1/5] virtio_net: Fix an unsafe reference to the page chain
On Fri, May 26, 2023 at 2:39 PM Jason Wang wrote: > > On Fri, May 26, 2023 at 1:46 PM Liang Chen wrote: > > > > "private" of buffer page is currently used for big mode to chain pages. > > But in mergeable mode, that offset of page could mean something else, > > e.g. when page_pool page is used instead. So excluding mergeable mode to > > avoid such a problem. > > If this issue happens only in the case of page_pool, it would be > better to squash it there. > > Thanks Sure, thanks! > > > > > Signed-off-by: Liang Chen > > --- > > drivers/net/virtio_net.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 5a7f7a76b920..c5dca0d92e64 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -497,7 +497,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info > > *vi, > > return NULL; > > > > page = (struct page *)page->private; > > - if (page) > > + if (!vi->mergeable_rx_bufs && page) > > give_pages(rq, page); > > goto ok; > > } > > -- > > 2.31.1 > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 1/5] virtio_net: Fix an unsafe reference to the page chain
On Fri, May 26, 2023 at 1:46 PM Liang Chen wrote: > > "private" of buffer page is currently used for big mode to chain pages. > But in mergeable mode, that offset of page could mean something else, > e.g. when page_pool page is used instead. So excluding mergeable mode to > avoid such a problem. If this issue happens only in the case of page_pool, it would be better to squash it there. Thanks > > Signed-off-by: Liang Chen > --- > drivers/net/virtio_net.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 5a7f7a76b920..c5dca0d92e64 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -497,7 +497,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info > *vi, > return NULL; > > page = (struct page *)page->private; > - if (page) > + if (!vi->mergeable_rx_bufs && page) > give_pages(rq, page); > goto ok; > } > -- > 2.31.1 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
