Re: [Devel] [PATCH vz7] ms/netfilter: bridge: make ebt_snat ARP rewrite writable

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] netfilter: ebt_snat backport notes misdescribe the tree (skb_make_writable)
--

commit 104d376b9788e858d4ac4df6973edf7b2248c98a
Author: Yiming Qian <yimingqian591@gmail.com>

ms/netfilter: bridge: make ebt_snat ARP rewrite writable

This commit adds a skb_ensure_writable() call in ebt_snat_tg() so that the
ARP sender hardware address range is writable before skb_header_pointer()
and skb_store_bits() are called. It is a backport of upstream commit
67ba971ae025. The code change looks correct. My only comment is on the
backport notes in the commit message.

> diff --git a/net/bridge/netfilter/ebt_snat.c b/net/bridge/netfilter/ebt_snat.c
> index 7dfbcdfc30e5..c9e229af0366 100644
> --- a/net/bridge/netfilter/ebt_snat.c
> +++ b/net/bridge/netfilter/ebt_snat.c

[ ... ]

> @@ -31,6 +31,9 @@ ebt_snat_tg(struct sk_buff *skb, const struct xt_action_param *par)
>  		const struct arphdr *ap;
>  		struct arphdr _ah;
>  
> +		if (skb_ensure_writable(skb, sizeof(_ah) + ETH_ALEN))
> +			return EBT_DROP;
> +
>  		ap = skb_header_pointer(skb, 0, sizeof(_ah), &_ah);
>  		if (ap == NULL)
>  			return EBT_DROP;

[Severity: Low]
This isn't a bug in the code, but the backport notes in the commit message
don't match the tree this patch applies to.

The notes say "the function here already has skb_make_writable(skb, 0)" and
"the rest of the file still uses skb_make_writable()". They also say that
63137bc5882a and the v5.3 conversion c1a831167901 are not present in this
tree.

In the tree as it stands with this patch applied, ebt_snat_tg() already
begins with:

    if (skb_ensure_writable(skb, 0))
        return EBT_DROP;

The sibling targets ebt_dnat_tg() and ebt_redirect_tg() also call
skb_ensure_writable(), and I found no skb_make_writable() left in
net/bridge/netfilter/. The patch only adds three lines, so the baseline
already had the skb_ensure_writable(skb, 0) call.

These notes look like they were written for the vz7 (3.10) version of the
patch. The statements "applies unchanged" and "63137bc5882a from Fixes: is
not here" may need to be corrected for this branch.

The mention of RHEL7 carrying the same change may also not belong in the
notes for this branch.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109-b4-prep-khorenko-vstor-154194-ebt_snat-arp-writable-v1-1-67df711251c0@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel