Re: [PATCH] [v2] net/mlx5e: fix a double-free in arfs_create_groups

[Simon Horman]

On Wed, Jan 10, 2024 at 09:23:24PM +0800, alexious@xxxxxxxxxx wrote:
> > On Mon, Jan 08, 2024 at 11:26:04PM +0800, Zhipeng Lu wrote:
> > > When `in` allocated by kvzalloc fails, arfs_create_groups will free
> > > ft->g and return an error. However, arfs_create_table, the only caller of
> > > arfs_create_groups, will hold this error and call to
> > > mlx5e_destroy_flow_table, in which the ft->g will be freed again.
> > > 
> > > Fixes: 1cabe6b0965e ("net/mlx5e: Create aRFS flow tables")
> > > Signed-off-by: Zhipeng Lu <alexious@xxxxxxxxxx>
> > > Reviewed-by: Simon Horman <horms@xxxxxxxxxx>
> > 
> > When working on netdev (and probably elsewhere)
> > Please don't include Reviewed-by or other tags
> > that were explicitly supplied by someone: I don't recall
> > supplying the tag above so please drop it.
> 
> I apologize, but it appears that you included a "reviewed-by" 
> tag along with certain suggestions for version 1 of this patch 
> in the first review email(about 6 days before). 

Yes, sorry. My statement above is not correct:
I now see that I did supply the tag.

> In response, after a short discussion, I followed some of 
> those suggestions and send this v2 patch.
> I referred to the "Dealing with tags" section in this KernelNewbies 
> tips: https://kernelnewbies.org/PatchTipsAndTricks and thought 
> that I should include that tag in v1 email to this v2 patch.
> So now I'm a little bit confused here: if the tag rule has changed 
> or I got some misunderstanding on existing rules? Your clarification 
> on this matter would be greatly appreciated.

I think in this case my statement above was incorrect,
and indeed the rule above is correct AFAIK

But it probably would have been best not to include the tag
in v2, because there were significant changes between v1 and v2.

> I'll send a new version of this patch after correcting the tag 
> issue and taking your suggestions into consideration.
> 
> Several comments below.

..

> > > @@ -883,7 +883,6 @@ void mlx5e_fs_init_l2_addr(struct mlx5e_flow_steering *fs, struct net_device *ne
> > >  void mlx5e_destroy_flow_table(struct mlx5e_flow_table *ft)
> > >  {
> > >  	mlx5e_destroy_groups(ft);
> > > -	kfree(ft->g);
> > >  	mlx5_destroy_flow_table(ft->t);
> > >  	ft->t = NULL;
> > 
> > Is the above still needed in some cases, and safe in all cases?
> 
> Well, in fact the kfree(ft->g) in mlx5e_destroy_flow_table causes 
> double frees in different functions such as fs_udp_create_table, 
> not only in arfs_create_groups. But you are right, with a more 
> detailed check I found that in some other functions, like 
> accel_fs_tcp_create_table, removing such free will cause memleak.
> So it could be a better idea to leave mlx5e_destroy_flow_table 
> as it used to be. And that follows the "one patch for one change" idea.

Right, I think it would be best to focus on fixing arfs_create_groups().
And making sure that neither leaks nor double frees occur. And I think
that at this point that includes ensuring ft->g is NULL if it has been freed.